Type check with the information from RapidsMeta #2888
Conversation
Signed-off-by: sperlingxx <[email protected]>
There was a problem hiding this comment.
Generally I think this is okay. My main problem is that it is not obvious to the end user that we changed the type. We have already been bitten by this with decimal and with timestamps where we do not explain to the user that decimal is disabled or that timestamps are disabled because the time zone is not UTC. I would like to avoid the same problem and ideally tag the types that we changed with a reason why we changed them.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/TypeChecks.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/CostBasedOptimizer.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/TypeChecks.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: sperlingxx <[email protected]>
Hi @revans2, I involved the reasons of type change via introducing a new class |
sameerz
added
the
task
sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsMeta.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/TypeChecks.scala
Outdated
Show resolved
Hide resolved
|
build |
Signed-off-by: sperlingxx <[email protected]>
|
build |
Fixes NVIDIA#2939. When the type-checking for expressions in `RapidsMeta` was changed in NVIDIA#2888 (i.e. commit 1a09322), `OffsetWindowFunctionMeta` was modified to include `input`, `offset`, and `default` expressions among its `childExprs`. Unfortunately, the commit neglected to replicate that change in `Spark311Shims` `OffsetWindowFunctionMeta`. The error manifests as a failure to do type-checks for `LEAD()`/`LAG()` window functions, with the error: `java.lang.AssertionError: assertion failed: Lead expected at least 3 but found 0`. This commit should remedy the situation, and allow `LEAD()`/`LAG()` to function correctly.
Fixes NVIDIA#2939. When the type-checking for expressions in `RapidsMeta` was changed in NVIDIA#2888 (i.e. commit 1a09322), `OffsetWindowFunctionMeta` was modified to include `input`, `offset`, and `default` expressions among its `childExprs`. Unfortunately, the commit neglected to replicate that change in `Spark311Shims` `OffsetWindowFunctionMeta`. The error manifests as a failure to do type-checks for `LEAD()`/`LAG()` window functions, with the error: `java.lang.AssertionError: assertion failed: Lead expected at least 3 but found 0`. This commit should remedy the situation, and allow `LEAD()`/`LAG()` to function correctly. Signed-off-by: Mithun RK <[email protected]>
Signed-off-by: sperlingxx [email protected]
Currently, during type tagging, we get the type information directly from what we have wrapped in the
RapidsMeta. It works well for most of expressions and plans. But for some special instances, such asTypedImperativeAggregate, we need to do type conversion based on the raw type. TakingTypedImperativeAggregateas an example, the GPU implementation introduces a different kind of internal aggregation buffer, whose data type is not same as the CPU counterpart.Therefore, in this PR, we modified the RapidsMetas and type checks, to do type check with the type information from the corresponding methods of RapidsMeta rather than raw data types of wrapped instances.