andygrove commented on code in PR #987:
URL: https://github.com/apache/datafusion-comet/pull/987#discussion_r1806679878
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -769,8 +769,12 @@ object QueryPlanSerde extends Logging with
ShimQueryPlanSerde with CometExprShim
val numBitsExpr = exprToProto(numBits, inputs, binding)
val dataType = serializeDataType(bloom_filter.dataType)
- if (childExpr.isDefined && numItemsExpr.isDefined &&
- numBitsExpr.isDefined && dataType.isDefined) {
+ if (childExpr.isDefined &&
+ child.dataType
+ .isInstanceOf[LongType] && // Spark 3.4 only supports Long, 3.5+
adds more types.
+ numItemsExpr.isDefined &&
+ numBitsExpr.isDefined &&
+ dataType.isDefined) {
Review Comment:
one minor nit (and no need to address for this PR, because this is a general
issue that we already have) is that we are testing for a multiple preconditions
here, and we fall back if any of them are false (which is correct) but we do
not let the user know the specific reason for the fallback, which makes
debugging more difficult.
I would eventually like to explore refactoring how we approach this and see
if we can add some utilities to make it easier to report fallback reasons, but
this is much lower priority than the performance & stability work for now.
--
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]
