Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -667,9 +667,12 @@ protected void runQuery(String sql) {
if (MetricRepo.isInit) {
MetricRepo.COUNTER_STATISTICS_INVALID_STATS.increase(1L);
}
String message = String.format("ColStatsData is invalid, skip analyzing. %s", colStatsData.toSQL(true));
LOG.warn(message);
throw new RuntimeException(message);
// Don't throw: keep writing the row into the statistics table so that the
Comment thread
codeDing18 marked this conversation as resolved.
Comment thread
codeDing18 marked this conversation as resolved.
// whole job still finishes. toColumnStatistic() will defensively convert
// this pattern to ColumnStatistic.UNKNOWN at read time, so the optimizer
// never sees the bogus numbers. See issue #64122.
LOG.warn("ColStatsData is invalid, will write to table but be treated as UNKNOWN at read time. {}",
colStatsData.toSQL(true));
}
// Update index row count after analyze.
if (this instanceof OlapAnalysisTask) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ public String toSQL(boolean roundByParentheses) {

public ColumnStatistic toColumnStatistic() {
try {
if (!isValid()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Avoid overflow in the newly activated ratio check — This guard now applies isValid() to manual cache publication and persisted-row reloads, but its ndv > 10 * count (and later nullCount * 10) arithmetic is signed long. A valid supported row such as row_count=1000000000000000000, ndv=1 makes 10 * count wrap negative, so both the immediate cache and reload become UNKNOWN and SHOW hides the row; the internal statistics fields are BIGINT and ALTER STATS accepts these numeric inputs. Please express both factor-of-ten comparisons without integral multiplication overflow and cover values above Long.MAX_VALUE / 10 through both conversion paths.

return ColumnStatistic.UNKNOWN;
}

ColumnStatisticBuilder columnStatisticBuilder = new ColumnStatisticBuilder(count);
columnStatisticBuilder.setNdv(ndv);
columnStatisticBuilder.setNumNulls(nullCount);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,11 @@ public static ColumnStatistic fromResultRowList(List<ResultRow> resultRows) {
* this function is used by analyze job and cbo job.
*/
public static ColumnStatistic fromResultRow(ResultRow row) {
ColStatsData statsData = new ColStatsData(row);
if (!statsData.isValid()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep retained invalid rows from flooding WARN on every read — isValid() logs on every false branch, so this new reader guard makes the intentionally persisted UNKNOWN representation noisy at every boundary: collection already validates and logs, syncColStats validates again, each follower validates again, and preheat/reload/refresh/SHOW repeat it for the retained row. Preheat alone can examine up to the default 500,000 cache entries. Please make the reader-side check quiet (or make validity testing side-effect-free) and emit the warning/metric once at the collection boundary.

return ColumnStatistic.UNKNOWN;
}

double count = Double.parseDouble(row.get(7));
ColumnStatisticBuilder columnStatisticBuilder = new ColumnStatisticBuilder(count);
double ndv = Double.parseDouble(row.getWithDefault(8, "0"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -295,15 +295,22 @@ private void doPreHeat() {
}

/**
* Refresh stats cache, invalidate cache if the new data is unknown.
* Refresh stats cache, publish UNKNOWN if the new data is unknown.
*/
public void syncColStats(ColStatsData data) {
StatsId statsId = data.statsId;
final StatisticsCacheKey k = new StatisticsCacheKey(statsId.catalogId, statsId.dbId, statsId.tblId,
statsId.idxId, statsId.colId);
ColumnStatistic columnStatistic = data.toColumnStatistic();
if (columnStatistic == ColumnStatistic.UNKNOWN) {
invalidateColumnStatsCache(k.catalogId, k.dbId, k.tableId, k.idxId, k.colName);
// Publish a blocking UNKNOWN instead of invalidating. Invalidation leaves the
// entry absent, so a concurrent get() can trigger the async loader which reads
// the previous (stale) row from the statistics table before the buffered
// insert commits, and that stale value would then survive until the next
// refresh. A put closes this window: readers hit UNKNOWN directly and no
// reload races with the flush.
updateColStatsCache(k.catalogId, k.dbId, k.tableId, k.idxId, k.colName,
ColumnStatistic.UNKNOWN);
} else {
putCache(k, columnStatistic);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,10 @@
import org.apache.doris.analysis.TableSample;
import org.apache.doris.catalog.Column;
import org.apache.doris.catalog.PrimitiveType;
import org.apache.doris.qe.StmtExecutor;

import com.google.common.collect.Lists;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.mockito.MockedConstruction;
import org.mockito.Mockito;

import java.util.List;

Expand Down Expand Up @@ -84,24 +81,9 @@ public void testNdvTooLarge() {
values.add("500");
values.add(null);
ResultRow row = new ResultRow(values);
List<ResultRow> result = Lists.newArrayList();
result.add(row);

try (MockedConstruction<StmtExecutor> mocked = Mockito.mockConstruction(StmtExecutor.class,
(mock, context) -> {
Mockito.when(mock.executeInternalQuery()).thenReturn(result);
})) {
BaseAnalysisTask task = new OlapAnalysisTask();
try {
task.runQuery("test");
} catch (Exception e) {
Assertions.assertEquals(e.getMessage(),
"ColStatsData is invalid, skip analyzing. "
+ "('id',10000,20000,30000,0,'col',null,100,1100,300,'min','max',400,'500',NULL)");
return;
}
Assertions.fail();
}
ColStatsData data = new ColStatsData(row);
Assertions.assertFalse(data.isValid());
Assertions.assertEquals(ColumnStatistic.UNKNOWN, data.toColumnStatistic());
}

@Test
Expand All @@ -123,23 +105,8 @@ public void testNdv0MinMaxExistsNullNotEqualCount() {
values.add("500");
values.add(null);
ResultRow row = new ResultRow(values);
List<ResultRow> result = Lists.newArrayList();
result.add(row);

try (MockedConstruction<StmtExecutor> mocked = Mockito.mockConstruction(StmtExecutor.class,
(mock, context) -> {
Mockito.when(mock.executeInternalQuery()).thenReturn(result);
})) {
BaseAnalysisTask task = new OlapAnalysisTask();
try {
task.runQuery("test");
} catch (Exception e) {
Assertions.assertEquals(e.getMessage(),
"ColStatsData is invalid, skip analyzing. "
+ "('id',10000,20000,30000,0,'col',null,500,0,300,'min','max',400,'500',NULL)");
return;
}
Assertions.fail();
}
ColStatsData data = new ColStatsData(row);
Assertions.assertFalse(data.isValid());
Assertions.assertEquals(ColumnStatistic.UNKNOWN, data.toColumnStatistic());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,6 @@ PhysicalResultSink
-- !ndv_row_invalid --
PhysicalResultSink
--hashJoin[INNER_JOIN broadcast] hashCondition=((region.r_regionkey = nation.n_regionkey)) otherCondition=()
----PhysicalOlapScan[region]
----PhysicalOlapScan[nation]
----PhysicalOlapScan[region]

4 changes: 2 additions & 2 deletions regression-test/suites/statistics/analyze_stats.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -2788,9 +2788,9 @@ PARTITION `p599` VALUES IN (599)
sql """alter table alter_test modify column id set stats ('row_count'='100', 'ndv'='0', 'num_nulls'='0.0', 'data_size'='2.69975443E8', 'min_value'='1', 'max_value'='2');"""
alter_result = sql """show column stats alter_test(id)"""
logger.info("show column alter_test(id) stats: " + alter_result)
assertEquals(1, alter_result.size())
assertEquals(0, alter_result.size())
alter_result = sql """show column cached stats alter_test(id)"""
assertEquals(1, alter_result.size())
assertEquals(0, alter_result.size())
sql """alter table alter_test modify column id set stats ('row_count'='100', 'ndv'='0', 'num_nulls'='100', 'data_size'='2.69975443E8', 'min_value'='1', 'max_value'='2');"""
alter_result = sql """show column stats alter_test(id)"""
logger.info("show column alter_test(id) stats: " + alter_result)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,12 @@ suite("test_analyze_all_null") {
sql """alter table invalidTest modify column col2 set stats ('row_count'='100', 'ndv'='0', 'num_nulls'='0.0', 'data_size'='3.2E8', 'min_value'='min', 'max_value'='max');"""
sql """alter table invalidTest modify column col3 set stats ('row_count'='100', 'ndv'='0', 'num_nulls'='100', 'data_size'='3.2E8', 'min_value'='min', 'max_value'='max');"""
result = sql """show column cached stats invalidTest"""
assertEquals(3, result.size())
assertEquals(2, result.size())

explain {
sql("memo plan select * from invalidTest")
contains "col1#0 -> ndv=100.0000"
contains "col2#1 -> ndv=0.0000"
contains "col2#1 -> unknown("
contains "col3#2 -> ndv=0.0000"
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

// Regression for issue #64122: ColStatsData.isValid() falsely rejects
// sampled column statistics when a column is (almost) all NULL.
//
// On a Unique-Key MoW table where column v is almost entirely NULL but
// has one surviving non-null value, sample analyze produces
// ndv=0 (estimated), min=max='x' (full-scan), nullCount != count
// which trips the second isValid() guard. Before the fix, runQuery()
// threw and aborted the whole analyze job; after the fix the row is
// written and toColumnStatistic() falls back to UNKNOWN at read time.
suite("test_analyze_sample_almost_all_null", "nonConcurrent") {

def wait_row_count_at_least = { db, table, threshold ->
// For Unique MoW the post-DELETE row count is non-trivial to predict
// exactly, so we just gate on "row count is reported and large enough",
// which is what we need to trigger the isValid() guard. count=0 would
// short-circuit isValid() and the issue would not reproduce.
def result = sql """show frontends;"""
def host
def port
for (int i = 0; i < result.size(); i++) {
if (result[i][8] == "true") {
host = result[i][1]
port = result[i][4]
}
}
def tokens = context.config.jdbcUrl.split('/')
def url = tokens[0] + "//" + host + ":" + port
connect(context.config.jdbcUser, context.config.jdbcPassword, url) {
sql """use ${db}"""
for (int i = 0; i < 120; i++) {
Thread.sleep(5000)
result = sql """SHOW DATA FROM ${table};"""
logger.info("SHOW DATA FROM ${table}: " + result)
// Sum the row-count column across all rows returned by SHOW DATA.
// Layout: rows are per-partition + a Total row at the end. The
// row-count column index is 4 (same assumption as the existing
// test_analyze_all_null suite).
def total = 0L
for (int r = 0; r < result.size(); r++) {
def v = result[r][4]
if (v == null) {
continue
}
try {
total += Long.parseLong(v.toString())
} catch (NumberFormatException ignored) {
// "Total" row may already be a formatted string; skip.
}
}
if (total >= threshold) {
return
}
}
throw new Exception("Row count report timeout for ${db}.${table}, "
+ "threshold=" + threshold + ", last result=" + result)
}
}

sql """drop database if exists regression_test_analyze_sample_almost_all_null"""
sql """create database regression_test_analyze_sample_almost_all_null"""
sql """use regression_test_analyze_sample_almost_all_null"""

// The suite mutates the cluster-global enable_auto_analyze, so it runs in the
// nonConcurrent group and restores the variable via setGlobalVarTemporary to
// avoid leaking the disabled state into other suites on failure.
setGlobalVarTemporary([enable_auto_analyze: false], {
sql """CREATE TABLE tbl_del_big (
k INT NOT NULL,
v VARCHAR(64) NULL
)
UNIQUE KEY(k)
DISTRIBUTED BY HASH(k) BUCKETS 64
PROPERTIES (
"replication_num" = "1",
"enable_unique_key_merge_on_write" = "true"
)
"""

// 2M rows with v=NULL; then 1 row with v='x' overwriting (k=1, NULL).
// DELETE half of the NULL rows so the surviving data set is ~1M rows,
// with exactly one non-null v value.
sql """INSERT INTO tbl_del_big SELECT number, NULL FROM numbers("number"="2000000")"""
sql """INSERT INTO tbl_del_big SELECT number * 64 + 1, 'x' FROM numbers("number"="1")"""
sql """DELETE FROM tbl_del_big WHERE k % 2 = 0 AND v IS NULL"""

wait_row_count_at_least("regression_test_analyze_sample_almost_all_null",
"tbl_del_big", 500000L)

sql """ANALYZE TABLE tbl_del_big WITH SAMPLE PERCENT 1 WITH SYNC"""

def result = sql """show column stats tbl_del_big"""

// k (NOT NULL) always produces valid sampled stats. Whether v also survives
// isValid() depends on whether the single 'x' row lands in one of the randomly
// chosen sample tablets (sampled -> ndv ~ 1, valid; not sampled -> ndv = 0 with
// full-scan min/max = 'x', invalid). So only assert on k and a loose row count.
assertTrue(result.size() >= 1)
assertTrue(result.any { it[0] == "k" })

// Deterministically construct the issue #64122 invalid pattern. SET STATS writes
// the row into the statistics table directly (no isValid check on that path) and
// syncColStats publishes UNKNOWN into the cache. Any later read also goes through
// ColumnStatistic.fromResultRow, whose isValid() guard returns UNKNOWN for
// ndv=0 + min/max!=null + nullCount!=count, so the optimizer must see unknown.
sql """ALTER TABLE tbl_del_big MODIFY COLUMN v SET STATS (
'row_count'='1000000', 'ndv'='0', 'num_nulls'='999999',
'data_size'='8000000', 'min_value'='x', 'max_value'='x')"""

explain {
sql("select * from tbl_del_big")
contains("planned with unknown column statistics")
}

explain {
sql("memo plan select * from tbl_del_big")
contains("v#1 -> unknown(")
}
})

sql """drop database if exists regression_test_analyze_sample_almost_all_null"""
Comment thread
codeDing18 marked this conversation as resolved.
}
Loading