github-actions[bot] commented on code in PR #69998:
URL: https://github.com/apache/doris/pull/69998#discussion_r4449359833
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShortCircuitQueryContext.java:
##########
@@ -55,7 +56,16 @@ public class ShortCircuitQueryContext {
public final int schemaVersion;
public final OlapTable tbl;
- private final long fileCacheQueryLimitBytes;
+ // Capture session options separately from planner-added query options.
Any change must
+ // replan with a fresh cacheID, since the BE also caches RuntimeState by
that ID.
+ private final TQueryOptions sessionQueryOptions;
+ // FE planning also consumes execution variables (e.g. default limits)
omitted from TQueryOptions.
+ private final Map<String, String> resultVariables;
+ // These globals can also be folded into output expressions and are not in
TQueryOptions.
+ private final String timeZone;
+ private final String lcTimeNames;
+ // The table object can be renamed and re-registered by REPLACE TABLE WITH
SWAP.
+ private final String tableName;
private final long partitionTopologyVersion;
public final OlapScanNode scanNode;
Review Comment:
[P1] Invalidate reuse when the selected materialized index changes. This
context captures `schemaVersion` from the table's base index, and `isReusable`
checks only that base version/topology, while `OlapScanNode.toThrift()` and the
selected tablets use `selectedIndexId` and its independent schema version.
Dropping or rebuilding a rollup can therefore leave the cached point plan
reusable with stale tablet IDs and send a base generation to BE for a rollup
descriptor. Capture/fingerprint the selected index ID and schema version from
the same scan snapshot and send that value in the lookup request.
##########
be/src/service/point_query_executor.cpp:
##########
@@ -231,22 +314,28 @@ RowCache* RowCache::instance() {
return ExecEnv::GetInstance()->get_row_cache();
}
-bool RowCache::lookup(const RowCacheKey& key, CacheHandle* handle) {
+bool RowCache::lookup(const RowCacheKey& key, const TabletSchemaSPtr& schema,
CacheHandle* handle) {
const std::string& encoded_key = key.encode();
auto* lru_handle = LRUCachePolicy::lookup(encoded_key);
if (!lru_handle) {
// cache miss
return false;
}
+ auto* cached_value = static_cast<RowCacheValue*>(value(lru_handle));
+ if (cached_value->schema.lock() != schema) {
+ release(lru_handle);
+ return false;
+ }
*handle = CacheHandle(this, lru_handle);
Review Comment:
[P1] Use the producing rowset schema for column-store fallback.
`validate_rowset_schema` permits an older rowset when the FE descriptor is
newer, so a rowset v1 can be read while `_tablet_schema` is v2 and the new UID
exists only in v2. The check at this block therefore skips the default path and
`seek_and_read_by_rowid(*_tablet_schema, ...)` asks the v1 segment for a column
it does not contain, returning an error instead of the descriptor default for
old rows. Check `rowset_schema->field_index(cid)` and pass the producing
`*rowset_schema` to the segment reader; add a pending-schema regression.
##########
be/test/service/point_query_exector_test.cpp:
##########
@@ -191,14 +209,503 @@ std::vector<TExpr> ReusableTestHelper::output_exprs =
[]() {
}();
TQueryOptions ReusableTestHelper::query_options = TQueryOptions();
-std::shared_ptr<TabletSchema> ReusableTestHelper::tablet_schema = []() {
- auto schema = std::make_shared<TabletSchema>();
- for (int i = 0; i < 11; ++i) {
-
schema->append_column(TabletColumn(FieldAggregationMethod::OLAP_FIELD_AGGREGATION_NONE,
- FieldType::OLAP_FIELD_TYPE_BIGINT));
- }
+std::shared_ptr<TabletSchema> ReusableTestHelper::tablet_schema() {
+ // Initialize after startup: append_column recognizes hidden columns using
string constants.
+ static auto schema = []() {
+ auto schema = std::make_shared<TabletSchema>();
+ for (const auto& slot :
ReusableTestHelper::create_descriptor_tablet().slotDescriptors) {
+ const auto& scalar = slot.slotType.types[0].scalar_type;
+ TColumn tcolumn;
+ tcolumn.__set_column_name(slot.colName);
+ tcolumn.__set_col_unique_id(slot.col_unique_id);
+ tcolumn.__set_is_allow_null(slot.nullIndicatorBit >= 0);
+ TColumnType type;
+ type.__set_type(scalar.type);
+ type.__set_len(scalar.len);
+ type.__set_precision(scalar.precision);
+ type.__set_scale(scalar.scale);
+ tcolumn.__set_column_type(type);
+ TabletColumn column;
+ column.init_from_thrift(tcolumn);
+ schema->append_column(column);
+ }
+ TabletColumn
delete_sign(FieldAggregationMethod::OLAP_FIELD_AGGREGATION_NONE,
+ FieldType::OLAP_FIELD_TYPE_TINYINT);
+ delete_sign.set_name(DELETE_SIGN);
+ delete_sign.set_unique_id(99);
+ schema->append_column(delete_sign);
+ schema->set_schema_version(0);
+ return schema;
+ }();
return schema;
-}();
+}
+
+TEST(PointQueryReusableTest, RejectInvalidAccessPaths) {
+ TColumnAccessPath access_path;
+ access_path.type = TAccessPathType::DATA;
+ TDataAccessPath data_path;
+ data_path.__set_path({"wrong_column", "leaf"});
+ access_path.__set_data_access_path(data_path);
+
+ auto init = [](const TDescriptorTable& desc_tbl) {
+ Reusable reusable;
+ return reusable.init(desc_tbl, ReusableTestHelper::output_exprs,
+ ReusableTestHelper::query_options,
+ *ReusableTestHelper::tablet_schema(), 2);
+ };
+
+ auto desc_tbl = ReusableTestHelper::create_descriptor_tablet();
+ ASSERT_TRUE(init(desc_tbl).ok());
+
+ desc_tbl.slotDescriptors[0].__set_all_access_paths({access_path});
+ auto status = init(desc_tbl);
+ ASSERT_FALSE(status.ok());
+ EXPECT_NE(status.to_string().find("column access paths"),
std::string::npos);
+ EXPECT_NE(status.to_string().find("k1"), std::string::npos);
+
+ desc_tbl = ReusableTestHelper::create_descriptor_tablet();
+ desc_tbl.slotDescriptors[0].__set_predicate_access_paths({access_path});
+ status = init(desc_tbl);
+ ASSERT_FALSE(status.ok());
+ EXPECT_NE(status.to_string().find("column access paths"),
std::string::npos);
+ EXPECT_NE(status.to_string().find("k1"), std::string::npos);
+}
+
+TEST(PointQueryReusableTest, RejectInvalidUidsWithoutAccessPaths) {
+ auto desc_tbl = ReusableTestHelper::create_descriptor_tablet();
+ auto init = [&](const TabletSchema& schema) {
+ Reusable reusable;
+ return reusable.init(desc_tbl, ReusableTestHelper::output_exprs,
+ ReusableTestHelper::query_options, schema, 2);
+ };
+ const auto& schema = *ReusableTestHelper::tablet_schema();
+ ASSERT_TRUE(init(schema).ok());
+ for (int uid : {-1, 1000}) {
+ desc_tbl.slotDescriptors[0].__set_col_unique_id(uid);
+ auto status = init(schema);
+ ASSERT_FALSE(status.ok());
+ EXPECT_NE(status.to_string().find("current column UID"),
std::string::npos);
+ }
+
+ // Light schema changes retain dropped UIDs for old rowsets, not current
row-store values.
+ TabletSchema replaced_schema;
+ for (const auto& column : schema.columns()) {
+ TabletColumn current = *column;
+ if (current.unique_id() == 0) {
+ current.set_unique_id(1000);
+ }
+ replaced_schema.append_column(current);
+ }
+ replaced_schema.merge_dropped_columns(schema);
+ ASSERT_GE(replaced_schema.field_index(0), 0);
+ desc_tbl.slotDescriptors[0].__set_col_unique_id(0);
+ ASSERT_FALSE(init(replaced_schema).ok());
+ desc_tbl.slotDescriptors[0].__set_col_unique_id(1000);
+ ASSERT_TRUE(init(replaced_schema).ok());
+}
+
+TEST(PointQueryReusableTest, AcceptFullStructRootPathFromOlderFe) {
+ TabletSchema schema;
+ schema.copy_from(*ReusableTestHelper::tablet_schema());
+ TabletColumn struct_column;
+ struct_column.set_name("typed");
+ struct_column.set_unique_id(100);
+ struct_column.set_type(FieldType::OLAP_FIELD_TYPE_STRUCT);
+ struct_column.set_is_nullable(true);
+ for (const auto* name : {"first", "second"}) {
+ TabletColumn child;
+ child.set_name(name);
+ child.set_type(FieldType::OLAP_FIELD_TYPE_STRING);
+ struct_column.add_sub_column(child);
+ }
+ schema.append_column(struct_column);
+
+ auto desc_tbl = ReusableTestHelper::create_descriptor_tablet();
+ desc_tbl.slotDescriptors[0].__set_colName("typed");
+ desc_tbl.slotDescriptors[0].__set_col_unique_id(100);
+ DataTypePtr full_type = std::make_shared<DataTypeStruct>(
+ DataTypes {std::make_shared<DataTypeString>(),
std::make_shared<DataTypeString>()},
+ Strings {"first", "second"});
+ desc_tbl.slotDescriptors[0].__set_slotType(full_type->to_thrift());
+ TColumnAccessPath root_path;
+ root_path.type = TAccessPathType::DATA;
+ TDataAccessPath root_data_path;
+ root_data_path.__set_path({"typed"});
+ root_path.__set_data_access_path(root_data_path);
+ desc_tbl.slotDescriptors[0].__set_all_access_paths({root_path});
+
+ auto init = [&](const TDescriptorTable& descriptor) {
+ Reusable reusable;
+ return reusable.init(descriptor, ReusableTestHelper::output_exprs,
+ ReusableTestHelper::query_options, schema, 2);
+ };
+ ASSERT_TRUE(init(desc_tbl).ok());
+
+ desc_tbl.slotDescriptors[0].__set_predicate_access_paths({root_path});
+ ASSERT_TRUE(init(desc_tbl).ok());
+
+ TColumnAccessPath first_path = root_path;
+ first_path.data_access_path.__set_path({"typed", "first"});
+ TColumnAccessPath second_path = root_path;
+ second_path.data_access_path.__set_path({"typed", "second"});
+ desc_tbl.slotDescriptors[0].__set_all_access_paths({first_path,
second_path});
+ desc_tbl.slotDescriptors[0].__set_predicate_access_paths({first_path});
+ ASSERT_TRUE(init(desc_tbl).ok());
+
+ for (const auto* suffix : {"OFFSET", "NULL"}) {
+ TColumnAccessPath meta_path;
+ meta_path.__set_type(TAccessPathType::META);
+ TMetaAccessPath meta;
+ meta.__set_path({"typed", suffix});
+ meta_path.__set_meta_access_path(meta);
+ desc_tbl.slotDescriptors[0].__set_all_access_paths({meta_path});
+ desc_tbl.slotDescriptors[0].__set_predicate_access_paths({meta_path});
+ ASSERT_TRUE(init(desc_tbl).ok());
+ // Older senders can carry META components in the data payload.
+ meta_path.__isset.meta_access_path = false;
+ TDataAccessPath legacy_meta;
+ legacy_meta.__set_path(meta.path);
+ meta_path.__set_data_access_path(legacy_meta);
+ desc_tbl.slotDescriptors[0].__set_all_access_paths({meta_path});
+ ASSERT_TRUE(init(desc_tbl).ok());
+ auto narrowed = desc_tbl;
+ DataTypePtr narrow_type = std::make_shared<DataTypeStruct>(
+ DataTypes {std::make_shared<DataTypeString>()}, Strings
{"first"});
+ narrowed.slotDescriptors[0].__set_slotType(narrow_type->to_thrift());
+ ASSERT_FALSE(init(narrowed).ok());
+ meta_path.data_access_path.__set_path({"another", suffix});
+ desc_tbl.slotDescriptors[0].__set_predicate_access_paths({meta_path});
+ ASSERT_FALSE(init(desc_tbl).ok());
+ }
+ desc_tbl.slotDescriptors[0].__set_all_access_paths({});
+ desc_tbl.slotDescriptors[0].__set_predicate_access_paths({});
+ ASSERT_TRUE(init(desc_tbl).ok());
+ auto changed = desc_tbl;
+ DataTypePtr narrow_type = std::make_shared<DataTypeStruct>(
+ DataTypes {std::make_shared<DataTypeString>()}, Strings {"first"});
+ changed.slotDescriptors[0].__set_slotType(narrow_type->to_thrift());
+ ASSERT_FALSE(init(changed).ok());
+ changed = desc_tbl;
+ changed.slotDescriptors[0].__set_nullIndicatorBit(-1);
+ ASSERT_FALSE(init(changed).ok());
+ changed = desc_tbl;
+
changed.slotDescriptors[0].slotType.types[0].struct_fields[0].__set_name("renamed");
+ ASSERT_FALSE(init(changed).ok());
+
+ // An unset legacy UID must not pass name/type validation while the
decoder still
+ // uses -1 instead of the row-store JSONB UID. Reject it before decoding
or fallback.
+ desc_tbl.slotDescriptors[0].__set_col_unique_id(-1);
+ ASSERT_FALSE(init(desc_tbl).ok());
+ desc_tbl.slotDescriptors[0].__set_all_access_paths({root_path});
+ desc_tbl.slotDescriptors[0].__set_predicate_access_paths({root_path});
+ ASSERT_FALSE(init(desc_tbl).ok());
+
+ // A drop/re-add can retain the name and type but not the row-store JSONB
UID.
+ desc_tbl.slotDescriptors[0].__set_col_unique_id(101);
+ ASSERT_FALSE(init(desc_tbl).ok());
+ desc_tbl.slotDescriptors[0].__set_col_unique_id(100);
+
+ TabletSchema replaced_schema;
+ for (const auto& column : schema.columns()) {
+ TabletColumn current = *column;
+ if (current.unique_id() == 100) {
+ current.set_unique_id(101);
+ }
+ replaced_schema.append_column(current);
+ }
+ replaced_schema.merge_dropped_columns(schema);
+ Reusable stale_reusable;
+ ASSERT_FALSE(stale_reusable
+ .init(desc_tbl, ReusableTestHelper::output_exprs,
+ ReusableTestHelper::query_options,
replaced_schema, 2)
+ .ok());
+
+ DataTypePtr narrowed_type = std::make_shared<DataTypeStruct>(
+ DataTypes {std::make_shared<DataTypeString>()}, Strings {"first"});
+ desc_tbl.slotDescriptors[0].__set_slotType(narrowed_type->to_thrift());
+ auto status = init(desc_tbl);
+ ASSERT_FALSE(status.ok());
+ EXPECT_NE(status.to_string().find("full stored column type"),
std::string::npos);
+
+ DataTypePtr reordered_fields = std::make_shared<DataTypeStruct>(
+ DataTypes {std::make_shared<DataTypeString>(),
std::make_shared<DataTypeString>()},
+ Strings {"second", "first"});
+ desc_tbl.slotDescriptors[0].__set_slotType(reordered_fields->to_thrift());
+ ASSERT_FALSE(init(desc_tbl).ok());
+}
+
+TEST(PointQueryReusableTest, DecodeFullStructWithLegacyMetaPaths) {
+ TabletSchema schema;
+ TabletColumn stored;
+ stored.set_name("s");
+ stored.set_unique_id(100);
+ stored.set_type(FieldType::OLAP_FIELD_TYPE_STRUCT);
+ for (const auto* name : {"first", "second"}) {
+ TabletColumn child;
+ child.set_name(name);
+ child.set_type(FieldType::OLAP_FIELD_TYPE_STRING);
+ stored.add_sub_column(child);
+ }
+ schema.append_column(stored);
+ schema.append_column(ReusableTestHelper::tablet_schema()->column(22));
+ DataTypePtr type = std::make_shared<DataTypeStruct>(
+ DataTypes {std::make_shared<DataTypeString>(),
std::make_shared<DataTypeString>()},
+ Strings {"first", "second"});
+ auto thrift_type = type->to_thrift();
+ TDescriptorTableBuilder builder;
+ TTupleDescriptorBuilder tuple;
+ tuple.add_slot(TSlotDescriptorBuilder()
+ .set_slotType(thrift_type)
+ .column_name("s")
+ .nullable(false)
+ .build());
+ tuple.build(&builder);
+ auto desc = builder.desc_tbl();
+ desc.slotDescriptors[0].__set_col_unique_id(100);
+ Struct value;
+ value.push_back(Field::create_field<TYPE_STRING>("one"));
+ value.push_back(Field::create_field<TYPE_STRING>("two"));
+ auto column = type->create_column();
+ column->insert(Field::create_field<TYPE_STRUCT>(value));
+ Block source;
+ source.insert({column->get_ptr(), type, "s"});
+ auto encoded = ColumnString::create();
+ JsonbSerializeUtil::block_to_jsonb(schema, source, *encoded, 1,
+
create_data_type_serdes(source.get_data_types()), {});
+ for (int encoding = 0; encoding < 4; ++encoding) {
+ TColumnAccessPath path;
+ path.__set_type(encoding < 2 ? TAccessPathType::DATA :
TAccessPathType::META);
+ if (encoding == 3) {
+ TMetaAccessPath meta;
+ meta.__set_path({"s", "NULL"});
+ path.__set_meta_access_path(meta);
+ } else {
+ TDataAccessPath data;
+ data.__set_path(encoding < 2 ? std::vector<std::string> {"s"}
+ : std::vector<std::string> {"s",
"OFFSET"});
+ path.__set_data_access_path(data);
+ }
+ desc.slotDescriptors[0].__set_all_access_paths(
+ encoding == 0 ? std::vector<TColumnAccessPath> {}
+ : std::vector<TColumnAccessPath> {path});
+ desc.slotDescriptors[0].__set_predicate_access_paths(
+ desc.slotDescriptors[0].all_access_paths);
+ Reusable reusable;
+ ASSERT_TRUE(reusable.init(desc, ReusableTestHelper::output_exprs,
+ ReusableTestHelper::query_options, schema)
+ .ok());
+ auto decoded = reusable.get_block();
+
ASSERT_TRUE(JsonbSerializeUtil::jsonb_to_block(reusable.get_data_type_serdes(),
*encoded,
+
reusable.get_col_uid_to_idx(), *decoded,
+
reusable.get_col_default_values(),
+
reusable.include_col_uids())
+ .ok());
+ ASSERT_EQ(decoded->rows(), 1);
+ EXPECT_EQ(decoded->get_by_position(0).column->compare_at(0, 0,
*column, 1), 0);
+ }
+}
+
+TEST(PointQueryReusableTest, AcceptFeAheadColumnsWithoutRemappingStaleUids) {
+ auto desc = ReusableTestHelper::create_descriptor_tablet();
+ desc.slotDescriptors[0].__set_colName("new_column");
+ desc.slotDescriptors[0].__set_col_unique_id(1000);
+ desc.slotDescriptors[0].__set_col_default_value("7");
+ const auto& schema = *ReusableTestHelper::tablet_schema();
+ Reusable reusable;
+ ASSERT_TRUE(reusable.init(desc, ReusableTestHelper::output_exprs,
+ ReusableTestHelper::query_options, schema, 1, 1)
+ .ok());
+ EXPECT_EQ(reusable.get_col_uid_to_idx().at(1000), 0);
+ EXPECT_EQ(reusable.get_col_default_values()[0], "7");
+ EXPECT_TRUE(reusable.missing_col_uids().contains(1000));
+ desc.slotDescriptors[0].__set_col_unique_id(-1);
+ Reusable unset;
+ ASSERT_FALSE(unset.init(desc, ReusableTestHelper::output_exprs,
+ ReusableTestHelper::query_options, schema)
+ .ok());
+}
+
+TEST(PointQueryReusableTest, RejectDroppedUidsWithoutSchemaHistory) {
+ auto desc = ReusableTestHelper::create_descriptor_tablet();
+ // The current schema no longer retains either the name or UID of the
dropped column.
+ desc.slotDescriptors[0].__set_colName("dropped");
+ desc.slotDescriptors[0].__set_col_unique_id(1000);
+ desc.slotDescriptors[0].__set_col_default_value("7");
+ TabletSchema schema;
+ schema.copy_from(*ReusableTestHelper::tablet_schema());
+ schema.set_schema_version(10);
+ for (int descriptor_version : {-1, 9, 10}) {
+ Reusable stale;
+ auto status = stale.init(desc, ReusableTestHelper::output_exprs,
+ ReusableTestHelper::query_options, schema, 1,
descriptor_version);
+ EXPECT_FALSE(status.ok());
+ EXPECT_NE(status.to_string().find("current column UID"),
std::string::npos);
+ }
+
+ // A demonstrably newer FE can still add columns and use defaults before
the next load.
+ Reusable added;
+ ASSERT_TRUE(added.init(desc, ReusableTestHelper::output_exprs,
+ ReusableTestHelper::query_options, schema, 1, 11)
+ .ok());
+ TabletSchema newer_rowset;
+ newer_rowset.copy_from(schema);
+ newer_rowset.set_schema_version(12);
+ EXPECT_FALSE(added.validate_rowset_schema(newer_rowset, schema).ok());
+
+ // Unknown BE generations cannot establish that the FE is ahead either.
+ schema.set_schema_version(-1);
+ Reusable unknown;
+ EXPECT_FALSE(unknown.init(desc, ReusableTestHelper::output_exprs,
+ ReusableTestHelper::query_options, schema, 1, 11)
+ .ok());
+}
+
+TEST(PointQueryReusableTest, MissingColumnsRequireDescriptorSlots) {
+ for (int row_store_mode = 0; row_store_mode < 3; ++row_store_mode) {
+ TabletSchema schema;
+ schema.copy_from(*ReusableTestHelper::tablet_schema());
+ if (row_store_mode != 0) {
+ TabletColumn
row_store(FieldAggregationMethod::OLAP_FIELD_AGGREGATION_NONE,
+ FieldType::OLAP_FIELD_TYPE_STRING);
+ row_store.set_name(BeConsts::ROW_STORE_COL);
+ row_store.set_unique_id(100);
+ schema.append_column(row_store);
+ if (row_store_mode == 1) {
+ TabletSchemaPB pb;
+ schema.to_schema_pb(&pb);
+ pb.add_row_store_column_unique_ids(1);
+ schema.init_from_pb(pb);
+ }
+ }
+ for (bool include_delete_sign : {false, true}) {
+ TDescriptorTableBuilder builder;
+ TTupleDescriptorBuilder tuple;
+ tuple.add_slot(TSlotDescriptorBuilder()
+ .type(TYPE_TINYINT)
+ .column_name("k1")
+ .nullable(true)
+ .build());
+ if (include_delete_sign) {
+ tuple.add_slot(
+ TSlotDescriptorBuilder()
+ .type(TYPE_TINYINT)
+ .column_name(DELETE_SIGN)
+
.nullable(schema.column(schema.delete_sign_idx()).is_nullable())
+ .build());
+ }
+ tuple.build(&builder);
+ auto desc = builder.desc_tbl();
+ desc.slotDescriptors[0].__set_col_unique_id(0);
+ if (include_delete_sign) {
+ desc.slotDescriptors[1].__set_col_unique_id(99);
+ }
+ Reusable reusable;
+ ASSERT_TRUE(reusable.init(desc, ReusableTestHelper::output_exprs,
+ ReusableTestHelper::query_options,
schema)
+ .ok());
+ EXPECT_EQ(reusable.delete_sign_idx(), include_delete_sign ? 1 :
-1);
+ EXPECT_EQ(reusable.missing_col_uids().contains(0), row_store_mode
!= 2);
+ EXPECT_EQ(reusable.missing_col_uids().contains(99),
+ include_delete_sign && row_store_mode != 2);
+ for (int uid : reusable.missing_col_uids()) {
+ EXPECT_TRUE(reusable.get_col_uid_to_idx().contains(uid));
+ }
+ }
+ }
+}
+
Review Comment:
[P1] Fix the new white-box test's access violations. The test directly
assigns `_tablet_schema`, `_reusable`, `_result_block`, `_row_read_ctxs` and
calls `_lookup_row_data()`, all private to `PointQueryExecutor`, then calls
`std::make_shared<BetaRowset>(...)` even though that constructor is protected.
This translation unit has no friend declaration or private-access macro, so the
added test target cannot compile; expose a test helper/friend or test through
the public API.
--
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]
