Return no-op instead of SQL error when ObservationLog has no valid entries

[pujitha24]

What this PR does / why we need it:

RegisterObservationLog in pkg/db/v1beta1/mysql/mysql.go and pkg/db/v1beta1/postgres/postgres.go builds an INSERT ... VALUES query by appending a (?, ?, ?, ?), placeholder group per MetricLog entry that has a non-empty TimeStamp, then strips the trailing comma with sqlQuery[0:len(sqlQuery)-1]. When every entry in the ObservationLog is skipped (empty MetricLogs, or every entry has an empty TimeStamp), no placeholder groups are appended, and the truncation instead chops the trailing space off "...VALUES ", producing a malformed statement. Prepare() then fails with a SQL syntax error, and that error is returned as-is by the ReportObservationLog gRPC handler (cmd/db-manager/v1beta1/main.go), so a call that should be a harmless no-op fails the RPC instead. This is per-RPC only — it does not crash or otherwise affect the db-manager process itself, just that one ReportObservationLog call.

Approach: Add an early if len(values) == 0 { return nil } guard right after the loop that builds values, before the query string is truncated, in both mysql.go and postgres.go. This is the primary fix the issue proposed. RegisterObservationLog's only caller passes its error straight through as the gRPC error, so returning nil when there is nothing to insert is a safe no-op with no skipped side effects.

Validation:

• Added TestRegisterObservationLogNoValidEntries to pkg/db/v1beta1/mysql/mysql_test.go and pkg/db/v1beta1/postgres/postgres_test.go, each registering an ObservationLog with a single MetricLog whose TimeStamp is empty (so it is skipped), asserting RegisterObservationLog returns nil.
• Confirmed these tests reproduce the exact bug: reverting only the two-line fix (keeping the new tests) makes both fail with Prepare SQL statement failed: ... call to Prepare 'INSERT INTO observation_logs (trial_name, time, metric_name, value) VALUES' query was not expected — sqlmock rejecting the malformed truncated query from the issue report. Re-applying the fix makes them pass.
• go build ./pkg/db/... — passes.
• go test ./pkg/db/v1beta1/mysql/... ./pkg/db/v1beta1/postgres/... — all tests pass, including the two new ones and the pre-existing TestRegisterObservationLog.
• gofmt -l on the changed files — clean.
• go vet ./pkg/db/v1beta1/mysql/... ./pkg/db/v1beta1/postgres/... — clean.
• golangci-lint run ./pkg/db/v1beta1/mysql/... ./pkg/db/v1beta1/postgres/... reports 7 pre-existing issues (errcheck on stmt.Close()/rows.Close(), staticcheck ST1005 on capitalized error strings); verified via git stash that all 7 are present identically on unmodified master and none are introduced by this change. Note: the locally available golangci-lint is v2.12.2, while this repo's make lint pins v1.64.7 — a version mismatch, disclosed here rather than resolved, since the flagged lines are all pre-existing and outside this diff.
• Did not run against a live MySQL or PostgreSQL instance; validation relies on this package's existing sqlmock-based unit test harness (github.com/DATA-DOG/go-sqlmock), the same harness the file's pre-existing RegisterObservationLog test uses.
• Branch is cut from upstream/master tip (dec5030ef9791add2a7224d8ba3cb9b699bf70da).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Checklist:

• Docs included if any changes are user facing

Fixes #2705

[github-actions]

🎉 Welcome to the Kubeflow Katib repo! 🎉

Thanks for opening your first PR! We're excited to have you onboard 🚀

Next steps:

• Our team will review your PR soon! cc @kubeflow/wg-automl-leads
• Check out the Contributing Guide and the Kubeflow Contributor Guide
• Join the Kubeflow Slack channels: https://www.kubeflow.org/docs/about/community/#kubeflow-slack-channels
• Join the AutoML & Training WG meetings: https://bit.ly/2PWVCkV

Feel free to ask questions in the comments. Thanks again for contributing! 🙏

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign electronic-waste for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[AdeshDeshmukh]

Hi @pujitha24 , thanks for jumping on this so quickly! For transparency — I filed #2705 and also have a fix ready for this (same approach as proposed in the issue), which I was holding until maintainer feedback before submitting a PR.

Since your PR is already up and looks correct, I'd suggest the maintainers move forward with it. I've left my review with a couple of small suggestions below.
Then follow up on that same PR with a review comment containing the suggestions (this is what actually adds value):

1. Add a test case for fully-empty MetricLogs (MetricLogs: []*api_pb.MetricLog{}) — currently only empty-timestamp entries are covered.
2. Optional nit: add a brief comment in mysql.go/postgres.go noting the no-op path deliberately skips Prepare/Exec.

[vjkumar2756]

@pujitha24
Great fix Returning early when no valid metric entries exist prevents malformed SQL syntax errors and avoids unnecessary DB execution.

Some Suggestions

1. Add nil Guard (mysql.go & postgres.go)
   If observationLog is nil, iterating over observationLog.MetricLogs will cause a runtime panic (nil pointer dereference). Consider checking observationLog == nil at the top of RegisterObservationLog:

   if observationLog == nil || len(observationLog.MetricLogs) == 0 {
       return nil
   }

[pujitha24]

Thanks both for the suggestions — pushed two follow-up commits addressing them:

• @AdeshDeshmukh: added TestRegisterObservationLogEmptyMetricLogs covering a fully-empty MetricLogs slice (separate from the empty-timestamp case), plus a short comment on the no-op path in both mysql.go and postgres.go.
• @vjkumar2756: added a nil check for observationLog at the top of RegisterObservationLog in both files, with a TestRegisterObservationLogNilObservationLog test confirming it returns nil instead of panicking.

Both packages' test suites pass locally with these changes.