perf: fast-path ASCII field matching - #2
Closed
ErikBPF wants to merge 1 commit into
Closed
Conversation
Avoid JVM case-table allocations and lookups for the common ASCII path.
Author
|
Superseded by apache#5602, which merged the broader fix on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of apache#174 and apache#5365.
Addresses the ASCII fast-path review findings in
apache#5365 (comment) and
apache#5365 (comment).
This targets
feature/delta-native-scanbecause the JVM case-table matcher iscurrently part of that unmerged branch.
Rationale for this change
names_equal_ignore_case_javacurrently allocates twoStrings and performsper-character case-table lookups for every comparison, including pure ASCII
field names. The matcher is used by the shared Parquet scan path.
Reviewer-reported release measurements found 2.3x to 12.0x end-to-end scan
slowdowns for representative many-file and many-column shapes. A separate
allocation probe reported 160,800 allocation or reallocation calls for 20,100
ASCII comparisons, versus zero with this guard.
What changes are included in this PR?
str::eq_ignore_ascii_casewhen both names are ASCII.How are these changes tested?
cargo test -p datafusion-comet: 316 passed, 19 ignored.cargo clippy -p datafusion-comet --lib -- -D warnings: clean.cargo fmt --all --check: clean.git diff --check: clean.Independent microbenchmark
Measured on a local server with
Rust 1.97.1, and the repository's default release profile (
optimized + debuginfo).The temporary probe compared the exact previous expression
(
java_lowercase(a, tables) == java_lowercase(b, tables)) withnames_equal_ignore_case_java. It used 200 already-lowercase ASCII names,the lower-triangular 20,100 comparisons repeated 50 times (1,005,000
comparisons per sample), five samples per path, and reported the median.
black_boxprotected inputs/results, and both paths were required to returnthe same match count.
The temporary benchmark was removed after measurement and is not part of the
production diff.
Results and limitations
The local release-mode matcher result independently confirms the expected
hot-path improvement. It is not an end-to-end
DataSourceExecbenchmark anddoes not independently measure allocation counts; those figures remain the
reviewers' measurements.
Non-ASCII comparisons still allocate and perform JVM case-table lookups. This
does not implement the separately suggested schema-wide lowercase hoisting.
AI assistance
OpenAI Codex assisted with implementation, test execution, benchmarking,
review, and this description. The contributor reviewed the change and remains
responsible for it.