Skip to content

add broker config options for sql log redaction#18604

Open
jadami10 wants to merge 8 commits into
apache:masterfrom
jadami10:jadami/oss-redact-query-sql
Open

add broker config options for sql log redaction#18604
jadami10 wants to merge 8 commits into
apache:masterfrom
jadami10:jadami/oss-redact-query-sql

Conversation

@jadami10
Copy link
Copy Markdown
Contributor

@jadami10 jadami10 commented May 28, 2026

This adds a new feature to support query redaction. By default, query logs are not redacted.

With literal_values, we use the the query fingerprint to only log the redacted query with no literal values. This is useful if folks still want the structure of the query without potentially leaking PII.

This also fixes a bug where query fingerprinting was modifying the AST in place and breaking queries. This closes #18426.
this was fixed yesterday in a better way.

The final option is full redaction. This is good if you want no SQL ending up in your logging system.

I tested all options internally on a QA cluster. We plan to stick with full redaction going forward.

@jadami10
Copy link
Copy Markdown
Contributor Author

i accidentally broke #18430.

@xiangfu0 @Jackie-Jiang anymore comments on this? I want to avoid more conflicts since this touches so many lines

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 28, 2026

Codecov Report

❌ Patch coverage is 34.09091% with 58 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.36%. Comparing base (c2561d4) to head (a81f5b4).
⚠️ Report is 25 commits behind head on master.

Files with missing lines Patch % Lines
...sthandler/BaseSingleStageBrokerRequestHandler.java 25.58% 29 Missing and 3 partials ⚠️
...requesthandler/MultiStageBrokerRequestHandler.java 8.33% 19 Missing and 3 partials ⚠️
.../org/apache/pinot/broker/querylog/QueryLogger.java 80.95% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18604      +/-   ##
============================================
+ Coverage     56.82%   64.36%   +7.54%     
- Complexity        7     1137    +1130     
============================================
  Files          2567     3336     +769     
  Lines        149066   206158   +57092     
  Branches      24103    32149    +8046     
============================================
+ Hits          84700   132690   +47990     
- Misses        57178    62802    +5624     
- Partials       7188    10666    +3478     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (?)
java-21 64.36% <34.09%> (+7.54%) ⬆️
temurin 64.36% <34.09%> (+7.54%) ⬆️
unittests 64.36% <34.09%> (+7.54%) ⬆️
unittests1 56.77% <ø> (-0.05%) ⬇️
unittests2 36.93% <34.09%> (?)

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.

@xiangfu0
Copy link
Copy Markdown
Contributor

The SQL redaction pass still appears incomplete for the single-stage RLS path. BaseSingleStageBrokerRequestHandler.applyRlsFilters() still logs LOGGER.debug("Applied RLS filters ... {}", query) with the raw SQL, so sqlRedaction=full or literal_values can still leak unredacted query text on real RLS-protected requests. That debug log needs to go through _queryLogger.redactQuery(query, requestContext.getQueryFingerprint()) as well.

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

4 participants