jayshrivastava commented on code in PR #21807:
URL: https://github.com/apache/datafusion/pull/21807#discussion_r3139930274
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -875,13 +875,8 @@ message PhysicalExprNode {
// Was date_time_interval_expr
reserved 17;
- // Unique identifier for this expression to do deduplication during
deserialization.
- // When serializing, this is set to a unique identifier for each combination
of
- // expression, process and serialization run.
- // When deserializing, if this ID has been seen before, the cached Arc is
returned
- // instead of creating a new one, enabling reconstruction of referential
integrity
- // across serde roundtrips.
- optional uint64 expr_id = 30;
+ // Was expr_id
+ reserved 30;
Review Comment:
If the `PhysicalExpr` has `expression_id` and the actual implementations
like `DynamicFilterPhysicalExpr` store it, it felt natural to have
`expression_id`s live in the implementor's proto (ie.
`PhysicalDynamicFilterNode`) rather than here, one level up. This is more
significant now since we don't dedupe every `PhysicalExpr` anymore. I thought
"If only dynamic filters have this, then it can live in the dynamic filter
proto".
It also avoids having to put `expr_id: None` everywhere.
I'll defer to your judgement but those were my thoughts.
--
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]
