Skip to content

fix(python): translate Polars struct field access - #10194

Open
danking wants to merge 3 commits into
developfrom
dk/polars-struct-field
Open

danking wants to merge 3 commits into
developfrom
dk/polars-struct-field

Conversation

@danking

@danking danking commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Translate Polars named struct field access to native Vortex get_item, preserving parent-struct nulls. The struct child is recursively translated and can be any supported expression.

This is a direct expression translation; it performs no eager evaluation and does not require literal arguments. Literal-specific PRs decode the scalar representation itself.

Validation: 1 targeted regression case(s) passed with this PR’s converter. With only polars_.py restored to the base revision and the same tests retained, all 1 failed with Polars ComputeError wrapping the unsupported-expression ValueError or NotImplementedError. The tests compare complete dataframes from Polars alone and a Vortex-backed Polars scan, and check explicit row expectations. No lint or broader suite was run.

Five independent parent-nullness tests explicitly construct Arrow arrays for a null parent, null inner parent, null outer parent, null middle parent across three levels, and combined parent/leaf nulls. Each specifies a concrete predicate and complete expected DataFrame, and confirms physical children retain masked values. The helper accepts an array, predicate, and expected DataFrame, evaluates the same filter in stock Polars and Vortex-backed Polars, and asserts both results equal the expected frame. These rewritten cases and lint have not been run.

@danking
danking marked this pull request as ready for review October 1, 2026 18:05
@danking danking added the changelog/fix A bug fix label Oct 1, 2026

Copy link
Copy Markdown
Contributor

Rebased onto develop (now includes #10187 and #10193) and fixed the Python (lint) failure (ty) in a separate commit.

The 5 test_polars_struct_field_null_* tests will keep failing Python (test) until #10402 merges. They expose a real reader bug: a file-scan filter on a field of a nullable struct ignored the struct's own nulls, so x.value >= 0 matched rows where x is null. #10402 fixes that in vortex-layout's StructReader. With it, this PR's full Python suite passes locally (416 passed). Once #10402 lands, this needs a develop merge or rebase to go green.


Generated by Claude Code

@codspeed

codspeed Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Merging this PR will improve performance by 10.66%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ 1 benchmark measured no execution time

Nothing ran under measurement, usually because the compiler removed the code under test. This result is not comparable, so it counts as unchanged.

Preventing compiler optimizations

⚠️ 14 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
✅ 2212 untouched benchmarks

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ WallTime threshold_avx512[8] 2.1 µs 1.9 µs +10.66%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing dk/polars-struct-field (0ba1a7f) with develop (d42e7c9)

Open in CodSpeed

@danking

danking commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

@claude rebase & fix lints!

@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @danking's task in 27s —— View job


Rebase & fix lints

  • Check the latest CI on this PR
  • Rebase onto develop: not possible here
  • Fix lints: nothing to fix

I couldn't rebase. This workflow is read-only: it can read code and post comments, but it can't rebase, commit or push. To get the rebase done, restate the task on an issue so the write workflow picks it up, or run it locally:

git fetch origin && git rebase origin/develop && git push --force-with-lease

Lints are already green. In the latest CI run (Linters and Tests #37851073451), Python (lint), Rust (lint), Rust (lint, no default), C/C++ (lint), lint-toml, Typo check and REUSE all pass. @robert3005's earlier commit fixed the ty failure.

One job fails: Python (test), at the "Pytest - Vortex" step. As @robert3005 noted above, that's expected. The 5 test_polars_struct_field_null_* tests expose a real reader bug in vortex-layout's StructReader: a filter on a field of a nullable struct ignores the struct's own nulls. #10402 fixes that bug. Rebasing now won't turn this job green unless #10402 has already merged into develop. Once it has, rebase (or merge develop in) and CI should pass.

Signed-off-by: Daniel King <dan@spiraldb.com>
@danking
danking force-pushed the dk/polars-struct-field branch from c70a23f to 6aef619 Compare October 12, 2026 19:26
Signed-off-by: Daniel King <dan@spiraldb.com>
@danking
danking enabled auto-merge (squash) October 12, 2026 19:35
Signed-off-by: Daniel King <dan@spiraldb.com>
@robert3005

Copy link
Copy Markdown
Contributor

I think this one is going to get fixed by @myrrc in core vortex and then here we will just have to translate the expression

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants