Make from/to optional in TBUCKET when Kibana timestamp filter is present#144057
Make from/to optional in TBUCKET when Kibana timestamp filter is present#144057felixbarny wants to merge 14 commits intoelastic:mainfrom
Conversation
Make numeric TBUCKET consume analyzer-injected timestamp bounds and add analysis/verification coverage for auto-bucketing and missing-range validation.
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
Hi @felixbarny, I've created a changelog YAML for you. |
🔍 Preview links for changed docs |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
...in/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/TBucket.java
Show resolved
Hide resolved
kkrik-es
left a comment
There was a problem hiding this comment.
Looks good, maybe add some csv tests too.
Made-with: Cursor
That's tricky. We can't add query dsl filters in csv tests. |
Made-with: Cursor
Move the rule after ResolveFunctions in the Initialize batch so that TBucket expressions already exist when timestamp bounds are injected.
sidosera
left a comment
There was a problem hiding this comment.
Thanks, Felix! This is great improvement
The numeric bucket null from/to check in resolveType() ran before ResolveTimestampBoundsAware could fill in bounds from the query filter. Move the check to postAnalysisVerification and wire up expression-level PostAnalysisVerificationAware in the Verifier.
📝 WalkthroughWalkthroughThis pull request enhances the TBUCKET function to support automatic bucket size derivation from Kibana's timestamp range filter. The TBucket class now implements TimestampBoundsAware to accept timestamp bounds from query context, adding methods needsTimestampBounds(), withTimestampBounds(), and postAnalysisVerification(). The Analyzer reorders initialization rules to resolve functions before timestamp bounds. The Verifier introduces a post-analysis verification pass for expressions implementing PostAnalysisVerificationAware. Documentation is updated across multiple reference and changelog files to reflect this capability. Tests verify the auto-bucketing behavior and updated error messages. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.41.0)x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java[] x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.javaComment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/changelog/144057.yaml`:
- Line 4: The YAML summary value under the key "summary" is unquoted and
contains a colon which breaks parsing; update the summary entry (the "summary"
key) to make the entire value a quoted scalar (e.g., wrap the whole string
including "Exmaple: `TBUCKET(100)`" in single or double quotes) so the colon is
treated as part of the string rather than a YAML separator.
In
`@x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/TBucket.java`:
- Around line 180-199: The current logic injects timestamp bounds when either
side is missing and also flags failures when either side is missing, which
allows a call like TBUCKET(10, from_only) to be auto-filled; update TBucket so
that needsTimestampBounds() only returns true when both from and to are null
(use from == null && to == null) and change the validation in
postAnalysisVerification to only fail when exactly one bound is provided (use an
exclusive-or check: (from == null) ^ (to == null)); keep references to the
existing methods needsTimestampBounds and postAnalysisVerification and the
constructor used in withTimestampBounds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d6b9f8f-45fb-4ff2-9868-d4ae3b15ea7b
📒 Files selected for processing (10)
docs/changelog/144057.yamldocs/reference/query-languages/esql/_snippets/functions/description/tbucket.mddocs/reference/query-languages/esql/_snippets/functions/parameters/tbucket.mddocs/reference/query-languages/esql/kibana/definition/functions/tbucket.jsondocs/reference/query-languages/esql/kibana/docs/functions/tbucket.mdx-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.javax-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.javax-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/TBucket.javax-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.javax-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
Make numeric TBUCKET consume analyzer-injected timestamp bounds and add analysis/verification coverage for auto-bucketing and missing-range validation.
The effect is that in Kibana, you can skip the from/to parameters and just use
TBUCKET(100), for example.