[BugFix] Propagate sample options to lake scan#71874
Conversation
levin-kitty
force-pushed
the
fix/lake-scan-sample-options
branch
from
46fe6b0 to
0ecd3de
Compare
levin-kitty
force-pushed
the
fix/lake-scan-sample-options
branch
from
0ecd3de to
cf390c3
Compare
|
@murphyatwork @wyb — could one of you take a look at this? It's been sitting since April with no review. The bug is real and not niche: The PR just completes the FE→BE propagation chain for lake, mirroring the existing Only gap I see is there's no test — @levin-kitty, worth adding one to lock the behavior in. |
|
Thanks for the feedback! I will take a look at how to add tests for this. |
levin-kitty
force-pushed
the
fix/lake-scan-sample-options
branch
from
cf390c3 to
359abe2
Compare
|
Hi, I rebased this PR onto the latest main and added two small regression tests as suggested.
Both focused tests passed locally. Could you please take a look when you have time? Thanks. |
levin-kitty
force-pushed
the
fix/lake-scan-sample-options
branch
2 times, most recently
from
8d56b30 to
5c761a7
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 4 / 4 (100.00%) file detail
|
|
[BE Incremental Coverage Report]✅ pass : 4 / 4 (100.00%) file detail
|
Signed-off-by: levin-kitty <levin.de.kitty@gmail.com>
Signed-off-by: levin-kitty <levin.de.kitty@gmail.com>
levin-kitty
force-pushed
the
fix/lake-scan-sample-options
branch
from
5c761a7 to
0de31e7
Compare
Module-risk briefing for Codex reviewThis is review focus, not a finding list. Validate against the diff. High-confidence risks
|
|
@xiangguangyxg @wangshisan @kevincai Rebased onto the latest main again and resolved the conflicts. The recent lake reader refactoring already included part of the sample_options propagation, so I removed the duplicated changes and kept only what is still needed. The focused FE and BE tests pass locally. Thanks! |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0de31e770f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (sample != null && sample.isUseSampling()) { | ||
| TTableSampleOptions sampleOptions = new TTableSampleOptions(); | ||
| msg.lake_scan_node.setSample_options(sampleOptions); | ||
| sample.toThrift(sampleOptions); | ||
| } |
There was a problem hiding this comment.
When enable_query_cache=true for shared-data queries, this makes the lake scan result depend on sample (method/seed/percent), but the query-cache digest is still built from OlapScanNode.toNormalForm, whose TNormalOlapScanNode has no sample fields and sampled scans are not marked uncacheable. Two otherwise identical lake queries with different SAMPLE options can therefore share the same cache entry, so the later query may read rows produced with the earlier sampling options. Please add the sample options to the normal form or disable query cache for sampled lake scans.
Useful? React with 👍 / 👎.
|
@levin-kitty Think about the codex review comment |
5 participants
Why I'm doing:
In shared-data mode, it looks like the TABLESAMPLE / SAMPLE('percent'='N') clause on lake tables doesn't actually take effect. From what I can see, the FE parses the clause correctly, but TLakeScanNode doesn't have a sample_options field, so the info never reaches the BE. So...
In my own test on a large table, ANALYZE SAMPLE went from about 2 minutes to about 8 seconds once sampling actually runs. I think this just brings shared-data to the same behavior that shared-nothing already has — please correct me if I'm missing context here.
What I'm doing:
Trying to complete the FE→BE propagation chain for sample_options on lake scans, mirroring the existing shared-nothing path.
As far as I can tell, everything downstream (SegmentIterator::_apply_data_sampling, BlockDataSample, PageDataSample) is already in place and didn't need changes. If I missed any path, please let me know.
This is my first PR to StarRocks — thanks in advance for the review, and happy to fix anything that needs changing.
Fixes #71873
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: