morrySnow commented on code in PR #66578:
URL: https://github.com/apache/doris/pull/66578#discussion_r3765757922
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/OlapAnalysisTask.java:
##########
@@ -270,55 +315,58 @@ protected void getSampleParams(Map<String, String>
params, long tableRowCount) {
params.put("preAggHint", "/*+PREAGGOPEN*/");
}
- // If table row count is less than the target sample row count, simple
scan the full table.
- if (tableRowCount <= targetSampleRows) {
+ // If the algorithm is FULL (table too small or sampled rows not
enough), scan the full table.
+ if (info.algorithm == AnalyzeSampleAlgorithm.FULL) {
params.put("scaleFactor", "1");
params.put("sampleHints", "");
- params.put("ndvFunction", "ROUND(NDV(`${colName}`) *
${scaleFactor})");
// For full table scan, use COUNT(1) for table row count.
params.put("rowCount", "COUNT(1)");
- params.put("rowCount2", "(SELECT COUNT(1) FROM cte1 WHERE
`${colName}` IS NOT NULL)");
- scanFullTable = true;
- return;
- }
- Pair<List<Long>, Long> sampleTabletsInfo = getSampleTablets();
- String tabletStr = sampleTabletsInfo.first.stream()
- .map(Object::toString)
- .collect(Collectors.joining(", "));
- String sampleHints = scanFullTable ? "" : String.format("TABLET(%s)",
tabletStr);
- params.put("sampleHints", sampleHints);
- long selectedRows = sampleTabletsInfo.second;
- long finalScanRows = selectedRows;
- double scaleFactor = scanFullTable ? 1 : (double) tableRowCount /
finalScanRows;
- params.put("scaleFactor", String.valueOf(scaleFactor));
-
- // If the tablets to be sampled are too large, use limit to control
the rows to read, and re-calculate
- // the scaleFactor.
- if (needLimit()) {
- finalScanRows = Math.min(targetSampleRows, selectedRows);
- if (col.isKey() && keyColumnSampleTooManyRows) {
- finalScanRows = MAXIMUM_SAMPLE_ROWS;
- }
- // Empty table doesn't need to limit.
- if (finalScanRows > 0) {
- scaleFactor = (double) tableRowCount / finalScanRows;
- params.put("limit", "limit " + finalScanRows);
- params.put("scaleFactor", String.valueOf(scaleFactor));
+ } else {
+ String tabletStr = info.sampleTablets.first.stream()
+ .map(Object::toString)
+ .collect(Collectors.joining(", "));
+ params.put("sampleHints", String.format("TABLET(%s)", tabletStr));
+ long selectedRows = info.sampleTablets.second;
+ long finalScanRows = selectedRows;
+ double scaleFactor = (double) tableRowCount / finalScanRows;
+ params.put("scaleFactor", String.valueOf(scaleFactor));
+
+ // If the tablets to be sampled are too large, use limit to
control the rows to read, and re-calculate
+ // the scaleFactor.
+ if (needLimit()) {
+ finalScanRows = Math.min(targetSampleRows, selectedRows);
+ if (col.isKey() && keyColumnSampleTooManyRows) {
+ finalScanRows = MAXIMUM_SAMPLE_ROWS;
+ }
+ // Empty table doesn't need to limit.
+ if (finalScanRows > 0) {
+ scaleFactor = (double) tableRowCount / finalScanRows;
+ params.put("limit", "limit " + finalScanRows);
+ params.put("scaleFactor", String.valueOf(scaleFactor));
+ }
}
}
- // Set algorithm related params.
- if (useLinearAnalyzeTemplate()) {
+ setSampleParamsByAlgorithm(params, tableRowCount, info.algorithm);
+ }
+
+ /**
+ * Set the algorithm related params. Must be called with the same
algorithm that the
+ * caller uses to pick the template, so the params always match the
template.
+ */
+ protected void setSampleParamsByAlgorithm(Map<String, String> params, long
tableRowCount,
+ AnalyzeSampleAlgorithm algorithm) {
+ if (algorithm == AnalyzeSampleAlgorithm.DUJ1) {
+ params.put("ndvFunction",
getNdvFunction(String.valueOf(tableRowCount)));
+ params.put("dataSizeFunction", getDataSizeFunction(col, true));
+ params.put("rowCount2", "(SELECT SUM(`count`) FROM cte1 WHERE
`col_value` IS NOT NULL)");
+ } else {
params.put("rowCount2", "(SELECT COUNT(1) FROM cte1 WHERE
`${colName}` IS NOT NULL)");
// For single unique key, use count as ndv.
if (isSingleUniqueKey()) {
params.put("ndvFunction", String.valueOf(tableRowCount));
Review Comment:
Behavior change for the FULL algorithm that is not mentioned in the PR: the
old small-table branch always set `ndvFunction` to `ROUND(NDV(`${colName}`) *
${scaleFactor})`, i.e. NDV computed over the actual full scan (pre-change code
lines 273-282). With this refactor, FULL flows into this else branch and
`isSingleUniqueKey()` now produces `ndvFunction =
String.valueOf(tableRowCount)` — the raw BE-reported tablet row count — while
`row_count` in the same stats row is `COUNT(1)` over the real scan. For a small
single-unique-key table (`tableRowCount <= targetSampleRows`), if the tablet
report lags behind the actual scan (the row-count report timing this PR is
itself about), `ndv` and `row_count` will disagree, whereas before they were
both derived from the same full scan. Consider keeping the full-scan NDV
expression for the FULL algorithm, or confirm this deviation is intended.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/OlapAnalysisTask.java:
##########
@@ -251,12 +250,58 @@ protected Pair<List<Long>, Long> getSampleTablets() {
return Pair.of(sampleTabletIds, selectedRows);
}
+ /**
+ * Result of the sample analyze algorithm decision: the chosen algorithm
and, when
+ * sampling is actually performed, the picked tablets used to build the
{@code TABLET(...)}
+ * hint. Kept local to the sampling flow so the long-lived task holds no
sampling state.
+ */
+ protected static class SampleCollectInfo {
+ public final AnalyzeSampleAlgorithm algorithm;
+ public final Pair<List<Long>, Long> sampleTablets;
+
+ public SampleCollectInfo(AnalyzeSampleAlgorithm algorithm,
Pair<List<Long>, Long> sampleTablets) {
+ this.algorithm = algorithm;
+ this.sampleTablets = sampleTablets;
+ }
+ }
+
+ /**
+ * Decide which statistics algorithm to use and, when sampling is actually
performed,
+ * collect the sample tablets. Call this before
+ * {@link #getSampleParams(Map, long, SampleCollectInfo)} so the params
are always filled
+ * according to the algorithm used to pick the template.
+ */
+ protected SampleCollectInfo getSampleCollectInfo(long tableRowCount) {
+ // Debug point used by tests to force the DUJ1 algorithm. Check it
before the
+ // row-count based FULL decision so tests are not affected by BE row
count report
+ // timing (e.g. a small table whose row count is not fully reported
yet).
+ if (DebugPointUtil.isEnable("OlapAnalysisTask.useDUJ1Template")) {
Review Comment:
This debug-point branch returns DUJ1 with whatever `getSampleTablets()`
picked, skipping the `sampleTablets.second < targetSampleRows` full-scan
fallback that the normal path applies two blocks below. If the sampled tablets
report 0 rows (tablet-level row counts can lag the table-level count at the
same moment — the exact reporting-timing flakiness this PR addresses),
`getSampleParams` then computes `scaleFactor = (double) tableRowCount /
finalScanRows` with `finalScanRows == 0`, which yields `Infinity`/`NaN`
substituted into the SQL, and an empty tablet list renders `TABLET()` with no
ids. Both produce an analyze SQL failure. Note this is not strictly test-only:
the regression test enables the debug point on all FEs and debug points can be
enabled on running clusters, so any concurrently running sample-analyze task on
an unreported table takes this path. Consider applying the same `selectedRows <
targetSampleRows` guard (fall back to LINEAR/FULL) here.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/OlapAnalysisTask.java:
##########
@@ -251,12 +250,58 @@ protected Pair<List<Long>, Long> getSampleTablets() {
return Pair.of(sampleTabletIds, selectedRows);
}
+ /**
+ * Result of the sample analyze algorithm decision: the chosen algorithm
and, when
+ * sampling is actually performed, the picked tablets used to build the
{@code TABLET(...)}
+ * hint. Kept local to the sampling flow so the long-lived task holds no
sampling state.
Review Comment:
Minor inconsistency between this doc claim and the code:
`getSampleTablets()` (called inside `getSampleCollectInfo`) still mutates the
instance fields
`keyColumnSampleTooManyRows`/`partitionColumnSampleTooManyRows`, and
`getSampleParams` reads them afterwards via
`needLimit()`/`useLinearAnalyzeTemplate()`. So the task does still hold
sampling state between the decision and the param filling, and the algorithm
decision is not fully self-contained in `SampleCollectInfo` — a second
invocation of `getSampleCollectInfo` on the same task instance would see
different results because of the leftover flags. Since the goal of this PR is
"one algorithm decision", capturing the two flags inside `SampleCollectInfo`
(returned alongside the algorithm) would make the decision truly single and
side-effect free.
##########
fe/fe-core/src/test/java/org/apache/doris/statistics/OlapAnalysisTaskTest.java:
##########
@@ -141,7 +143,11 @@ public void testKeyColumnUseLimitAndNot() {
OlapAnalysisTask olapAnalysisTask = Mockito.spy(new
OlapAnalysisTask());
Mockito.doReturn(new ResultRow(Lists.newArrayList("1",
"2"))).when(olapAnalysisTask).collectMinMax();
-
Mockito.doNothing().when(olapAnalysisTask).getSampleParams(ArgumentMatchers.any(),
ArgumentMatchers.anyLong());
+
Mockito.doNothing().when(olapAnalysisTask).getSampleParams(ArgumentMatchers.any(),
+ ArgumentMatchers.anyLong(), ArgumentMatchers.any());
+ Mockito.doReturn(new
OlapAnalysisTask.SampleCollectInfo(AnalyzeSampleAlgorithm.LINEAR,
+ Pair.of(Lists.newArrayList(1L, 2L), 100L)))
+
.when(olapAnalysisTask).getSampleCollectInfo(ArgumentMatchers.anyLong());
Mockito.doReturn(true).when(olapAnalysisTask).useLinearAnalyzeTemplate();
Review Comment:
Dead stub: after this refactor `doSample()` chooses the template from
`collectInfo.algorithm` and no longer calls `useLinearAnalyzeTemplate()` —
`getSampleCollectInfo` is mocked here, so this stub has no effect on the
asserted SQL (the second run in this test already dropped its corresponding
stub). Consider removing it to keep the test honest.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/OlapAnalysisTask.java:
##########
@@ -123,16 +122,17 @@ protected void doSample() {
long tableRowCount = info.indexId == -1
? tbl.getRowCount()
: ((OlapTable) tbl).getRowCountForIndex(info.indexId, false);
- getSampleParams(params, tableRowCount);
+ SampleCollectInfo collectInfo = getSampleCollectInfo(tableRowCount);
+ getSampleParams(params, tableRowCount, collectInfo);
StringSubstitutor stringSubstitutor = new StringSubstitutor(params);
String sql;
- if (useLinearAnalyzeTemplate()) {
- sql = stringSubstitutor.replace(LINEAR_ANALYZE_TEMPLATE);
- } else {
+ if (collectInfo.algorithm == AnalyzeSampleAlgorithm.DUJ1) {
Review Comment:
Test coverage gap: the original bug was exactly a template/params mismatch,
and the tests now cover LINEAR→LINEAR and DUJ1→DUJ1 at the doSample SQL level
(`testKeyColumnUseLimitAndNot`) plus the decision itself
(`testGetSampleCollectInfo`), but the third combination — FULL algorithm
rendering LINEAR-style params into the LINEAR template — is only covered
piecemeal (params in `testGetSampleParams`, template choice never asserted for
FULL). A doSample-level case with `getSampleCollectInfo` returning `FULL` +
null tablets would assert the final SQL is the LINEAR template with
`COUNT(1)`/full-scan params, closing the loop on the exact failure class this
PR fixes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
