Skip to content

fix(common): avoid mutating query AST during fingerprinting#18443

Open
wolfkill wants to merge 1 commit into
apache:masterfrom
wolfkill:fix/query-fingerprint-ast-copy
Open

fix(common): avoid mutating query AST during fingerprinting#18443
wolfkill wants to merge 1 commit into
apache:masterfrom
wolfkill:fix/query-fingerprint-ast-copy

Conversation

@wolfkill
Copy link
Copy Markdown

@wolfkill wolfkill commented May 8, 2026

Summary

  • Make QueryFingerprintVisitor return copied Calcite calls instead of mutating SqlNode operands in place
  • Preserve structural operands such as hints and join metadata while visiting data expressions
  • Add regression coverage proving generateFingerprint leaves the original SqlNode unchanged for later planning

Fixes #18426

Tests

  • git diff --check
  • JAVA_HOME=/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home PATH=/opt/homebrew/opt/openjdk@21/bin:$PATH ./mvnw -pl pinot-common -am -Dtest=org.apache.pinot.common.utils.request.QueryFingerprintUtilsTest -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false test

The targeted Maven run completed with 37 QueryFingerprintUtilsTest tests passing and checkstyle enabled across the affected reactor modules.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 12, 2026

Codecov Report

❌ Patch coverage is 93.02326% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.64%. Comparing base (4863cdc) to head (c24998a).
⚠️ Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
.../common/utils/request/QueryFingerprintVisitor.java 93.02% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18443      +/-   ##
============================================
- Coverage     63.65%   63.64%   -0.01%     
  Complexity     1735     1735              
============================================
  Files          3254     3254              
  Lines        199446   199434      -12     
  Branches      30977    30972       -5     
============================================
- Hits         126953   126934      -19     
- Misses        62356    62358       +2     
- Partials      10137    10142       +5     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-21 63.64% <93.02%> (-0.01%) ⬇️
temurin 63.64% <93.02%> (-0.01%) ⬇️
unittests 63.64% <93.02%> (-0.01%) ⬇️
unittests1 55.69% <93.02%> (-0.02%) ⬇️
unittests2 34.96% <39.53%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

query fingerprinting mutates the SqlNode in place and breaks queries

2 participants