Repository navigation
fix: escape single quotes in the seed dataset SQL literal - #1109
xubenqiang wants to merge 1 commit into
Conversation
SeedReader interpolated the dataset URI into DuckDB string literals without escaping embedded single quotes, so a local seed under a path such as o'brien/ failed with a parser error. Quote the URI through one helper that doubles embedded quotes and use it in all four query sites (count, describe, and both read branches). Fixes NVIDIA-NeMo#1102
Linked Issue CheckIssue #1102 has not been triaged yet. A maintainer needs to review New or reopened PRs without a triaged issue are closed. Check the |
|
Thank you for your submission! We ask that you sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text: I have read the DCO document and I hereby sign the DCO. You can retrigger this bot by commenting recheck in this Pull Request. Posted by the DCO Assistant Lite bot. |
 [Medium impact] The PR affects seed dataset queries used in data-generation workflows. The PR appears safe to merge, with a non-blocking error-message text regression. Findings
Fix with agent prompt### Issue 1
packages/data-designer-engine/src/data_designer/engine/resources/seed_reader.py:96
The two error messages replace `🛑` with the garbled text `馃洃`. Users see these characters when a seed directory is missing or an agent rollout reader gets a non-local path. Restore `🛑` here and in the message at line 783.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryThis PR fixes seed paths containing single quotes by using
Reviews (1) · Last reviewed commit: "fix: escape single quotes in the seed da..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| resolved_root_path = Path(runtime_path).expanduser().resolve() | ||
| if not resolved_root_path.is_dir(): | ||
| raise SeedReaderConfigError(f"🛑 Seed source directory '{resolved_root_path}' does not exist.") | ||
| raise SeedReaderConfigError(f"馃洃 Seed source directory '{resolved_root_path}' does not exist.") |
There was a problem hiding this comment.
The two error messages replace 🛑 with the garbled text 馃洃. Users see these characters when a seed directory is missing or an agent rollout reader gets a non-local path. Restore 🛑 here and in the message at line 783.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-engine/src/data_designer/engine/resources/seed_reader.py
Line: 96
Comment:
**Error messages show garbled text**
The two error messages replace `🛑` with the garbled text `馃洃`. Users see these characters when a seed directory is missing or an agent rollout reader gets a non-local path. Restore `🛑` here and in the message at line 783.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Fixes #1102
What changed
SeedReaderbuilt DuckDB SQL by interpolatingself.get_dataset_uri()into single-quoted string literals, so a local seed under a path containing'(for exampleo'brien/seed.parquet) ended the literal early and DuckDB raised a parser error.The URI now goes through one
_as_sql_literal()helper that doubles embedded single quotes, and all four sites use it:get_seed_dataset_size,get_column_namesand both branches ofbuild_dataset_read_query.Files:
packages/data-designer-engine/src/data_designer/engine/resources/seed_reader.pypackages/data-designer-engine/tests/engine/resources/test_seed_reader.pyTesting
The new test writes a parquet seed under
o'brien/, attaches aLocalFileSeedReaderand checksget_seed_dataset_size()/get_column_names():With the source change reverted it fails with the reported error:
The one remaining failure in the file,
test_filesystem_seed_reader_uses_injected_filesystem_provider, also fails on the unmodified checkout here because it expectsremote-root/alpha.txtwhile Windows joins with a backslash; it is unrelated to this change.AI assistance
Prepared with an AI coding agent (DeepSeek Harness / deepseek-flash) and verified with the runs above.