github-actions[bot] commented on code in PR #61300:
URL: https://github.com/apache/doris/pull/61300#discussion_r2985456080
##########
be/src/core/field.cpp:
##########
@@ -724,17 +732,31 @@ std::strong_ordering Field::operator<=>(const Field& rhs)
const {
case PrimitiveType::TYPE_IPV4:
return get<TYPE_IPV4>() <=> rhs.get<TYPE_IPV4>();
case PrimitiveType::TYPE_FLOAT:
- return get<TYPE_FLOAT>() < rhs.get<TYPE_FLOAT>() ?
std::strong_ordering::less
- : get<TYPE_FLOAT>() == rhs.get<TYPE_FLOAT>() ?
std::strong_ordering::equal
- :
std::strong_ordering::greater;
+ switch (Compare::compare(get<TYPE_FLOAT>(), rhs.get<TYPE_FLOAT>())) {
+ case -1:
+ return std::strong_ordering::less;
+ case 0:
+ return std::strong_ordering::equal;
+ case 1:
+ return std::strong_ordering::greater;
+ default:
+ LOG(FATAL) << "unexpected float compare result";
+ }
case PrimitiveType::TYPE_TIMEV2:
Review Comment:
Minor: The `default:` branch of this inner `switch` does `LOG(FATAL)` but
has no `break` or `return` statement. If execution somehow continued past
`LOG(FATAL)`, it would fall through the outer `case TYPE_FLOAT` into `case
TYPE_TIMEV2`. While `CompareFloat` only returns {-1, 0, 1} making the `default`
unreachable, consider adding `__builtin_unreachable()` after the `LOG(FATAL)`
to make the intent explicit and silence potential compiler warnings about
control reaching the end of a non-void function. Same applies to the
`TYPE_DOUBLE` case below.
##########
be/src/service/point_query_executor.cpp:
##########
@@ -386,21 +386,43 @@ void PointQueryExecutor::print_profile() {
Status PointQueryExecutor::_init_keys(const PTabletKeyLookupRequest* request) {
SCOPED_TIMER(&_profile_metrics.init_key_ns);
+ const auto& schema = _tablet->tablet_schema();
+ // Point query is only supported on merge-on-write unique key tables.
+ DCHECK(schema->keys_type() == UNIQUE_KEYS &&
_tablet->enable_unique_key_merge_on_write());
+ if (schema->keys_type() != UNIQUE_KEYS ||
!_tablet->enable_unique_key_merge_on_write()) {
+ return Status::InvalidArgument(
+ "Point query is only supported on merge-on-write unique key
tables, "
+ "tablet_id={}",
+ _tablet->tablet_id());
+ }
// 1. get primary key from conditions
- std::vector<OlapTuple> olap_tuples;
- olap_tuples.resize(request->key_tuples().size());
+ _row_read_ctxs.resize(request->key_tuples().size());
+ // get row cursor and encode keys
for (int i = 0; i < request->key_tuples().size(); ++i) {
const KeyTuple& key_tuple = request->key_tuples(i);
- for (const std::string& key_col : key_tuple.key_column_rep()) {
- olap_tuples[i].add_value(key_col);
+ if (UNLIKELY(cast_set<size_t>(key_tuple.key_column_literals_size()) !=
+ schema->num_key_columns())) {
+ return Status::InvalidArgument(
Review Comment:
**Critical: Rolling upgrade incompatibility.**
This code exclusively reads `key_column_literals` and will fail with
`InvalidArgument` when receiving requests from an old FE that only populates
`key_column_rep`. During rolling upgrades (FE upgraded before or after BE), all
point queries will break.
Suggested fix: add a fallback path:
```cpp
if (key_tuple.key_column_literals_size() > 0) {
// new typed path (current code)
} else if (key_tuple.key_column_rep_size() > 0) {
// legacy string-based path (old init_scan_key + from_tuple logic)
} else {
return Status::InvalidArgument(...);
}
```
Also, the FE side (`PointQueryExecutor.java`) should populate **both**
`key_column_rep` and `key_column_literals` during a transition period so that
old BEs can still function.
##########
gensrc/proto/internal_service.proto:
##########
@@ -353,6 +353,11 @@ message PFetchArrowFlightSchemaResult {
message KeyTuple {
repeated string key_column_rep = 1;
+ // Each entry is a serialized TExprNode (Thrift binary protocol)
representing
+ // a literal value for the corresponding key column. When present, BE uses
+ // DataType::get_field(TExprNode) to extract typed Field values directly,
+ // avoiding the string-parsing path used by key_column_rep.
+ repeated bytes key_column_literals = 2;
}
Review Comment:
The proto definition correctly adds `key_column_literals` as a new field
(tag 2) alongside the existing `key_column_rep` (tag 1), preserving wire
compatibility. However, the BE and FE code changes do not maintain runtime
backward compatibility — see comments on `point_query_executor.cpp` and
`PointQueryExecutor.java`.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/PointQueryExecutor.java:
##########
@@ -205,9 +210,39 @@ void addKeyTuples(
SlotRef columnSlot = left.unwrapSlotRef();
columnExpr.put(columnSlot.getColumnName(), right);
}
- // add key tuple in keys order
+ // Serialize each literal expr as TExprNode bytes for typed value
transfer.
+ // BE deserializes the TExprNode and uses DataType::get_field() to
extract
+ // typed Field values directly, avoiding string parsing.
+ TSerializer serializer = new TSerializer();
for (Column column :
shortCircuitQueryContext.scanNode.getOlapTable().getBaseSchemaKeyColumns()) {
-
kBuilder.addKeyColumnRep(columnExpr.get(column.getName()).getStringValue());
+ Expr literalExpr = columnExpr.get(column.getName());
+ // Ensure the literal type matches the column type for proper
TExprNode
+ // deserialization on BE side. Prepared statement parameters may
have
+ // mismatched types (e.g., setBigDecimal for INT column produces a
+ // DecimalLiteral, but BE expects INT_LITERAL for INT columns).
+ if (literalExpr instanceof LiteralExpr) {
+ Type colType = column.getType();
+ if (!colType.equals(literalExpr.getType())
+ && !colType.matchesType(literalExpr.getType())) {
+ try {
+ literalExpr = LiteralExpr.create(
+ ((LiteralExpr) literalExpr).getStringValue(),
colType);
+ } catch (org.apache.doris.common.AnalysisException e) {
+ throw new TException("Failed to re-type literal for
key column "
+ + column.getName() + ": " + e.getMessage(), e);
+ }
+ }
+ }
+ TExpr texpr = ExprToThriftVisitor.treeToThrift(literalExpr);
+ // For point queries, key column values are always simple literals
+ // (CastExpr no-ops are already stripped by treeToThrift).
+ Preconditions.checkState(texpr.getNodesSize() == 1,
+ "Expected single TExprNode for key column literal of " +
column.getName()
+ + ", got " + texpr.getNodesSize());
+ TExprNode exprNode = texpr.getNodes().get(0);
+ byte[] serialized = serializer.serialize(exprNode);
+ kBuilder.addKeyColumnLiterals(
+ com.google.protobuf.ByteString.copyFrom(serialized));
}
Review Comment:
**Critical: Must also populate `key_column_rep` for backward compatibility.**
During rolling upgrades, old BEs will only read `key_column_rep` (the string
path). The new FE must populate both fields so that old BEs continue to
function:
```java
// For backward compatibility during rolling upgrades
kBuilder.addKeyColumnRep(((LiteralExpr) literalExpr).getStringValue());
// New typed path
kBuilder.addKeyColumnLiterals(ByteString.copyFrom(serialized));
```
The old `key_column_rep` population can be removed in a future version after
all clusters have upgraded.
--
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]
