peter-toth commented on code in PR #10473:
URL: https://github.com/apache/datafusion/pull/10473#discussion_r1650118774
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -43,18 +45,35 @@ const CSE_PREFIX: &str = "__common_expr";
/// Identifier that represents a subexpression tree.
///
-/// Note that the current implementation contains:
-/// - the `Display` of an expression (a `String`) and
-/// - the identifiers of the childrens of the expression
-/// concatenated.
-///
/// An identifier should (ideally) be able to "hash", "accumulate", "equal"
and "have no
/// collision (as low as possible)"
-///
-/// Since an identifier is likely to be copied many times, it is better that
an identifier
-/// is small or "copy". otherwise some kinds of reference count is needed.
String
-/// description here is not such a good choose.
-type Identifier = String;
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+struct Identifier<'n> {
+ hash: u64,
Review Comment:
Sure, and exactly as you say. Added a comment in
https://github.com/apache/datafusion/commit/ee61224f1a948c38ac99ae53d9d426bb69da9548.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
