Repository navigation
Preserve Python literals in generated catalog queries - #73
zack-dev-cm wants to merge 3 commits into
Conversation
![neuralink-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4876455?s=60&v=4){kind=small}
There was a problem hiding this comment.
No material correctness, security, API, test-gap, or error-handling issues in this diff. formatPythonLiteral correctly switches generated catalog code from unescaped interpolation/Array.toString() to Python literals (JSON-escaped strings, True/False/None, bracketed lists), which fixes the reported argument-splitting and empty-list bugs without changing unary-null or SQL formatting. Unsafe integral JS numbers are emitted with toExponential() so Python does not parse a different integer value; non-finite numbers and non-JSON objects fail loudly with TypeError as documented. The new exporter/tsc round-trip tests cover escaping, nested lists, signed zero, large floats, selected columns, and local Parquet execution.
Posted by the code review bot. This is an automated review, not a maintainer approval.
Serialize scalar and nested list values, escape string arguments, and preserve large floating-point values through Python token generation. Add exporter-to-generator execution and local Parquet regressions.
abc6188 to
0159422
Compare
There was a problem hiding this comment.
No material issues in this change. Filter/name/column values are now emitted as real Python literals, which fixes the argument-splitting and silent-corruption cases (lists, empty lists, quotes, booleans, null, unsafe integrals) without altering unary-null handling or SQL predicate formatting. JSON.stringify for strings, True/False/None, recursive lists, -0.0, and toExponential() for non-safe integers are the right round-trip choices after JSON/JS transport; unsupported objects and non-finite numbers fail closed with TypeError. The new exporter/tsc/exec tests cover the behavioral surface, including nested lists, escaping, and a local Parquet query.
Posted by the code review bot. This is an automated review, not a maintainer approval.
| return Object.is(value, -0) ? '-0.0' : String(value) | ||
| } | ||
| if (Array.isArray(value)) return '[' + value.map(formatPythonLiteral).join(', ') + ']' | ||
| throw new TypeError('Cannot generate a Python literal for this filter value') |
There was a problem hiding this comment.
@zack-dev-cm instead of throwing a TypeError in formatPythonLiteral for unsupported values (which would break the table page render), could the snippet emit a placeholder like # cannot render this filter value instead?
There was a problem hiding this comment.
Sure, corrected. Unsupported filter values now display # cannot render this filter value without breaking the table page. Regression tests and all required CI checks pass. Thanks for catching this!
There was a problem hiding this comment.
No material correctness, security, API, or test-gap issues in this diff. Rendering filter values with explicit Python literals (and JSON.stringify for names/strings) fixes the previous argument-splitting and escaping bugs, and unsupported or non-finite values fail closed with a non-executable placeholder instead of emitting a wrong Filter call. Number formatting matches the stated JSON/JS transport limits (including unsafe integrals via exponential tokens and signed zero), and the new exporter/tsc regressions cover the behavioral changes.
Posted by the code review bot. This is an automated review, not a maintainer approval.
This branch has not been deployed
2 participants
Catalog filters containing lists, booleans, quotes or backslashes can generate Python that changes the value or the number of
Filterarguments. For example, an integer list becomes several arguments, while an empty list drops the value argument.Render transported scalar/list values as Python literals and escape string arguments consistently. Unsafe integral JavaScript numbers use exponential floating-point tokens to preserve their transported numeric value. Filter operators, unary-null behavior and SQL formatting are preserved.
Unsupported runtime objects and nonfinite numbers, including unsupported values nested inside lists, produce the comment-only snippet
# cannot render this filter value. The table page continues rendering, and the example cannot silently omit an unsupported filter. The 39 regressions exercise the real exporter and compiled TypeScript generator, execute supported snippets against a recording API, and include local Parquet queries.The branch includes current main and the merged credential correction from #74. The PR diff contains only the generator, its filter-value type and the regression module.
Validation:
TypeErroron the previous PR revision and pass with this correction.This preserves the value available after JSON/JavaScript transport; it cannot recover precision or distinguish
1.0from1after that information has already been lost.