github-actions[bot] commented on code in PR #64262:
URL: https://github.com/apache/doris/pull/64262#discussion_r3393303842
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java:
##########
@@ -900,6 +902,9 @@ public static List<String>
analyzeRowStoreColumns(Map<String, String> properties
return null;
}
properties.remove(PROPERTIES_ROW_STORE_COLUMNS);
+ if (!value.trim().isEmpty()) {
Review Comment:
This makes every existing non-empty `row_store_columns` property fail
analysis. That property is already used by the point-query row-store suites,
partial-update row-store suite, and row-store docs, so this PR breaks existing
CREATE/ALTER TABLE row-store-column workflows even when `row_store_only` is not
requested. If subset row-store is being removed, it needs a
compatibility/deprecation path, updated tests, and a release note; otherwise
keep the existing `row_store_columns` behavior and validate `row_store_only`
separately.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckAfterRewrite.java:
##########
@@ -65,10 +70,53 @@ public Rule build() {
checkUnexpectedExpression(plan);
checkMetricTypeIsUsedCorrectly(plan);
checkMatchIsUsedCorrectly(plan);
+ checkRowStoreOnlyComplexQuery(plan);
return null;
}).toRule(RuleType.CHECK_ANALYSIS);
}
+ private void checkRowStoreOnlyComplexQuery(Plan plan) {
+ if (shouldBlockRowStoreOnlyComplexQuery(plan)) {
+ throw new
AnalysisException(ROW_STORE_ONLY_COMPLEX_QUERY_ERROR_MSG);
+ }
+ }
+
+ private boolean shouldBlockRowStoreOnlyComplexQuery(Plan plan) {
+ if (!Config.enable_row_store_only_complex_query_block) {
+ return false;
+ }
+
+ boolean hasRowStoreOnlyOlapScan =
plan.collect(OlapScan.class::isInstance).stream()
+ .map(OlapScan.class::cast)
+ .anyMatch(scan -> scan.getTable().rowStoreOnly());
+ if (!hasRowStoreOnlyOlapScan) {
+ return false;
+ }
+
+ if (plan.anyMatch(node -> node instanceof LogicalJoin)) {
+ return true;
+ }
+ if (!plan.anyMatch(node -> node instanceof LogicalAggregate)) {
+ return false;
+ }
+ if (plan.anyMatch(node -> node instanceof LogicalSort || node
instanceof LogicalTopN)) {
+ return true;
+ }
+
+ return plan.collect(node -> node instanceof LogicalAggregate).stream()
+ .map(node -> (LogicalAggregate<?>) node)
+ .anyMatch(aggregate -> !isCountStarOnly(aggregate));
+ }
+
+ private boolean isCountStarOnly(LogicalAggregate<?> aggregate) {
+ if (!aggregate.getGroupByExpressions().isEmpty() ||
aggregate.getSourceRepeat().isPresent()) {
+ return false;
+ }
Review Comment:
This whitelist accepts any single aggregate output that contains a
`count(*)`, not only a pure count-star aggregate. For example, `SELECT count(*)
+ sum(v) FROM row_store_only_table` has one output expression and no group
by/sort, so `anyMatch` finds the `Count(*)` child and the aggregate is allowed
even though it also reads `v`. The check should verify the output is exactly
`Count(*)` (or an alias over it), not just that `Count(*)` appears somewhere in
the expression tree.
##########
gensrc/proto/olap_file.proto:
##########
@@ -586,6 +587,7 @@ message TabletSchemaCloudPB {
// Persisted TStorageFormat (V2=2, V3=3); see
TabletSchemaPB::storage_format.
optional TabletStorageFormatPB storage_format = 37;
Review Comment:
Adding the CloudPB field is not enough because FE never populates it on
cloud tablet creation. `CloudInternalCatalog.createTabletMetaBuilder` still
only receives `storeRowColumn` and only calls
`schemaBuilder.setStoreRowColumn(...)`; normal cloud create, cloud
rollup/schema-change, and cloud restore call sites have no `rowStoreOnly`
argument. A cloud table created with `row_store_only=true` will persist the FE
table property, but the meta-service/BE tablet schema gets the default `false`,
so it stores normal column pages instead of the new layout. Please thread
`tbl.rowStoreOnly()` through the cloud builder and set this field.
--
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]
