Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: sqlite query config escaping #6

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

lampajr
Copy link

@lampajr lampajr commented Feb 21, 2024

Fixes https://issues.redhat.com/browse/RHOAIENG-3496

Description

Fixes an issue introduced with the string escaping:

I experienced he following issue by starting the mlmd server (image quay.io/opendatahub/mlmd-grpc-server:main-ec2c7b1) together with sqlite configuration.

ARNING: Logging before InitGoogleLogging() is written to STDERR                                                                                                                          [15:31:43]
F0221 14:31:43.422607     1 metadata_store_server_main.cc:611] Check failed: absl::OkStatus() == status (OK vs. INTERNAL: Error when executing query: no such column: mlmd.Dataset query:  SELECT `id`, `name`, `version`, `description`,         `input_type`, `output_type` FROM `Type`  WHERE name IN (mlmd.Dataset, mlmd.Model, mlmd.Metrics, mlmd.Statistics) AND version IS NULL AND type_kind = 1; ) MetadataStore cannot be created with the given connection config.
*** Check failure stack trace: ***

The sqlite config I used is the same used in model registry: https://github.com/opendatahub-io/model-registry/blob/main/test/config/ml-metadata/conn_config.pb for testing purposes.

Since I am not expert on the codebase I don't know if this fix is going to revert the issue fixed with #2

How Has This Been Tested?

Build local image and run model registry test using the sqlite configuration.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@isinyaaa
Copy link

Hi, Andrea, thanks for fixing that, it had not occurred to me to test it using sqlite.

Since I am not expert on the codebase I don't know if this fix is going to revert the issue fixed with #2

I'm afraid it actually does, now, I think the fix is exactly as you did, but you should instead apply the quotes around SqliteMetadataSource::EscapeString on ml_metadata/metadata_store/sqlite_metadata_source.cc.

@lampajr lampajr requested a review from isinyaaa February 21, 2024 20:12
@lampajr lampajr marked this pull request as ready for review February 21, 2024 20:12
@lampajr
Copy link
Author

lampajr commented Feb 21, 2024

@isinyaaa I tried locally and the sqlite configs looks working, model-registry tests are working using image built from this branch

@lampajr lampajr requested review from dhirajsb and tarilabs February 22, 2024 08:12
@lampajr lampajr merged commit 94ae1e9 into opendatahub-io:master Feb 22, 2024
1 check passed
@lampajr lampajr deleted the fix_sqlite_query branch February 22, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants