github-actions[bot] commented on code in PR #69998:
URL: https://github.com/apache/doris/pull/69998#discussion_r4446411634
##########
be/src/service/point_query_executor.cpp:
##########
@@ -135,6 +137,48 @@ Status Reusable::init(const TDescriptorTable& t_desc_tbl,
const std::vector<TExp
_runtime_state->set_query_options(query_options);
RETURN_IF_ERROR(DescriptorTbl::create(_runtime_state->obj_pool(),
t_desc_tbl, &_desc_tbl));
_runtime_state->set_desc_tbl(_desc_tbl);
+ for (const auto* slot : tuple_desc()->slots()) {
+ // Every decoder/fallback map below uses the descriptor UID, even
without access paths.
+ // Retained dropped columns are still indexed by UID, but cannot serve
current row data.
+ const int column_idx =
+ slot->col_unique_id() < 0 ? -1 :
schema.field_index(slot->col_unique_id());
Review Comment:
[P1] Preserve the light-schema default fallback for FE-ahead columns
This early UID check rejects any descriptor column absent from the BE tablet
schema before `_lookup_row_data` can reach its existing light-schema branch
(lines 636-651), which inserts the FE default/NULL when FE has already added a
column but the tablet has not caught up. The existing
`test_uuid_point_query_light_schema_change` explicitly exercises this state and
expects the default UUID/NULL for old rows. Distinguish a missing new column
from a stale positive UID that matches a different current column, or defer
this validation so the documented fallback remains reachable.
##########
be/src/service/point_query_executor.cpp:
##########
@@ -135,6 +137,48 @@ Status Reusable::init(const TDescriptorTable& t_desc_tbl,
const std::vector<TExp
_runtime_state->set_query_options(query_options);
RETURN_IF_ERROR(DescriptorTbl::create(_runtime_state->obj_pool(),
t_desc_tbl, &_desc_tbl));
_runtime_state->set_desc_tbl(_desc_tbl);
+ for (const auto* slot : tuple_desc()->slots()) {
+ // Every decoder/fallback map below uses the descriptor UID, even
without access paths.
+ // Retained dropped columns are still indexed by UID, but cannot serve
current row data.
+ const int column_idx =
+ slot->col_unique_id() < 0 ? -1 :
schema.field_index(slot->col_unique_id());
+ if (column_idx < 0 ||
schema.is_dropped_column(schema.column(column_idx)) ||
+ !iequal(schema.column(column_idx).name(), slot->col_name())) {
+ return Status::InvalidArgument(
+ "Short-circuit point query requires a current column UID,
slot: {}, UID: {}",
+ slot->col_name(), slot->col_unique_id());
+ }
+ const auto& all_paths = slot->all_access_paths();
+ const auto& predicate_paths = slot->predicate_access_paths();
+ if (all_paths.empty() && predicate_paths.empty()) {
Review Comment:
[P1] Validate descriptor types even without access paths
The new type check is skipped by the `all_paths.empty() &&
predicate_paths.empty()` branch, which is the normal point-query shape. During
a light schema change the FE can keep the same UID while changing a
STRUCT/complex layout before a BE tablet's row-store payload catches up. The
SerDe is built from the descriptor type, and
`DataTypeStructSerDe::read_one_cell_from_jsonb` then deserializes the old
binary blob using the new child count. Compare type/nullability for every slot,
or defer the point query until the BE schema generation is compatible.
##########
be/src/service/point_query_executor.cpp:
##########
@@ -135,6 +137,48 @@ Status Reusable::init(const TDescriptorTable& t_desc_tbl,
const std::vector<TExp
_runtime_state->set_query_options(query_options);
RETURN_IF_ERROR(DescriptorTbl::create(_runtime_state->obj_pool(),
t_desc_tbl, &_desc_tbl));
_runtime_state->set_desc_tbl(_desc_tbl);
+ for (const auto* slot : tuple_desc()->slots()) {
+ // Every decoder/fallback map below uses the descriptor UID, even
without access paths.
+ // Retained dropped columns are still indexed by UID, but cannot serve
current row data.
+ const int column_idx =
+ slot->col_unique_id() < 0 ? -1 :
schema.field_index(slot->col_unique_id());
+ if (column_idx < 0 ||
schema.is_dropped_column(schema.column(column_idx)) ||
+ !iequal(schema.column(column_idx).name(), slot->col_name())) {
+ return Status::InvalidArgument(
+ "Short-circuit point query requires a current column UID,
slot: {}, UID: {}",
+ slot->col_name(), slot->col_unique_id());
+ }
+ const auto& all_paths = slot->all_access_paths();
Review Comment:
[P1] Scope the reusable BE cache by tablet schema
A prepared UUID is reused for every bound key, but
`LookupConnectionCache::get(uuid)` accepts the first `Reusable` before
resolving or validating the request's tablet. That object carries SerDes, UID
maps, and row-store include/missing sets initialized from the first tablet.
When tablets are at different light-schema/row-store generations, a later key
can therefore decode its row-store JSONB or column-store fallback with another
tablet's layout and return defaults or wrong values. Include tablet/schema
generation in the cache key or validate and rebuild the reusable on a mismatch.
##########
be/src/service/point_query_executor.cpp:
##########
@@ -135,6 +137,48 @@ Status Reusable::init(const TDescriptorTable& t_desc_tbl,
const std::vector<TExp
_runtime_state->set_query_options(query_options);
RETURN_IF_ERROR(DescriptorTbl::create(_runtime_state->obj_pool(),
t_desc_tbl, &_desc_tbl));
_runtime_state->set_desc_tbl(_desc_tbl);
+ for (const auto* slot : tuple_desc()->slots()) {
+ // Every decoder/fallback map below uses the descriptor UID, even
without access paths.
+ // Retained dropped columns are still indexed by UID, but cannot serve
current row data.
+ const int column_idx =
+ slot->col_unique_id() < 0 ? -1 :
schema.field_index(slot->col_unique_id());
+ if (column_idx < 0 ||
schema.is_dropped_column(schema.column(column_idx)) ||
+ !iequal(schema.column(column_idx).name(), slot->col_name())) {
+ return Status::InvalidArgument(
+ "Short-circuit point query requires a current column UID,
slot: {}, UID: {}",
+ slot->col_name(), slot->col_unique_id());
+ }
+ const auto& all_paths = slot->all_access_paths();
+ const auto& predicate_paths = slot->predicate_access_paths();
+ if (all_paths.empty() && predicate_paths.empty()) {
+ continue;
+ }
+ auto is_data_column_path = [&](const TColumnAccessPath& path) {
+ return path.type == TAccessPathType::DATA &&
path.__isset.data_access_path &&
Review Comment:
[P2] Accept safe legacy META paths with a full descriptor type
The compatibility gate accepts only DATA paths, so an older FE descriptor
carrying a full type plus a META path such as `[v, OFFSET]` or `[v, NULL]` is
rejected. `NestedColumnPruning` emits these META paths for `length`/null
checks, while row-store JSONB decoding ignores the path list and can safely
decode the complete type. Permit META paths only after verifying the descriptor
is the complete stored type, or normalize them at the compatibility boundary,
so rolling FE upgrades do not fail point queries.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShortCircuitQueryContext.java:
##########
@@ -109,29 +118,38 @@ public ShortCircuitQueryContext(Planner planner,
Queriable analzyedQuery) throws
this.cacheID = UUID.randomUUID();
this.scanNode = olapScanNode;
this.tbl = this.scanNode.getOlapTable();
+ this.tableName = this.tbl.getName();
this.schemaVersion = this.tbl.getBaseSchemaVersion();
this.partitionTopologyVersion = this.tbl.getPartitionTopologyVersion();
this.analzyedQuery = analzyedQuery;
}
@VisibleForTesting
- ShortCircuitQueryContext(OlapTable tbl, int schemaVersion, long
fileCacheQueryLimitBytes) {
+ ShortCircuitQueryContext(OlapTable tbl, int schemaVersion, SessionVariable
sessionVariable) {
this.planner = null;
this.serializedDescTable = ByteString.EMPTY;
this.serializedOutputExpr = ByteString.EMPTY;
this.serializedQueryOptions = ByteString.EMPTY;
this.cacheID = UUID.randomUUID();
this.tbl = tbl;
+ this.tableName = tbl.getName();
this.schemaVersion = schemaVersion;
- this.fileCacheQueryLimitBytes = fileCacheQueryLimitBytes;
+ this.sessionQueryOptions = sessionVariable.toThrift().deepCopy();
+ this.timeZone = sessionVariable.getTimeZone();
+ this.lcTimeNames = sessionVariable.getLcTimeNames();
this.partitionTopologyVersion = tbl.getPartitionTopologyVersion();
this.scanNode = null;
this.analzyedQuery = null;
}
public boolean isReusable(ConnectContext ctx) {
- return this.tbl.getBaseSchemaVersion() == this.schemaVersion
- && this.fileCacheQueryLimitBytes ==
ctx.getSessionVariable().fileCacheQueryLimitBytes
+ DatabaseIf database = this.tbl.getDatabase();
+ return !this.tbl.isDropped
+ && database != null &&
database.getTableNullable(this.tableName) == this.tbl
+ && this.timeZone.equals(ctx.getSessionVariable().getTimeZone())
+ &&
this.lcTimeNames.equals(ctx.getSessionVariable().getLcTimeNames())
+ && this.tbl.getBaseSchemaVersion() == this.schemaVersion
+ &&
this.sessionQueryOptions.equals(ctx.getSessionVariable().toThrift())
Review Comment:
[P1] Invalidate the cached plan for FE-only result-affecting variables
`isReusable` compares `TQueryOptions`, timezone, and locale, but
`SessionVariable.toThrift()` omits plan variables such as
`block_encryption_mode` and `decimal_overflow_scale` whenever
`enable_condition_cache` is false. `AesEncrypt` binds its implicit mode literal
from the current session, and decimal `Divide`/`Multiply` derive output scale
from the current setting. A prepared point query can therefore change one of
these settings between EXECUTEs while this check still passes; the direct path
reuses the old serialized expression and field metadata. Fingerprint all
`affectQueryResultInPlan` values (or force a replan) and add toggle regressions.
--
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]
