feat: support Spark-compatible abs math function part 2 - ANSI mode #18828
Conversation
github-actions
bot
added
sqllogictest
| // hardcode target partitions so plans are deterministic | ||
| .with_target_partitions(4); | ||
| .with_target_partitions(4) | ||
| .with_enable_ansi_mode(true); |
There was a problem hiding this comment.
Turn on ANSI mode here so that sqllogictest show overflow error. We need a different way to toggle ANSI mode.
There was a problem hiding this comment.
Can we just set it like so instead?
datafusion/datafusion/sqllogictest/test_files/cte.slt
Lines 45 to 47 in f1ecacc
Instead of doing it globally
There was a problem hiding this comment.
typically we set a param in slt and then return the original value
statement ok
set datafusion.execution.batch_size = 2;
run stuff ....
statement ok
set datafusion.execution.batch_size = 8192;
hsiang-c
force-pushed
the
spark_ansi_abs
branch
from
88ca183 to
94cf2a8
Compare
| // hardcode target partitions so plans are deterministic | ||
| .with_target_partitions(4); | ||
| .with_target_partitions(4) | ||
| .with_enable_ansi_mode(true); |
There was a problem hiding this comment.
Can we just set it like so instead?
datafusion/datafusion/sqllogictest/test_files/cte.slt
Lines 45 to 47 in f1ecacc
Instead of doing it globally
| eval_legacy_mode!(Float64, -0.0f64, 0.0f64); | ||
| } | ||
|
|
||
| macro_rules! eval_ansi_mode { |
There was a problem hiding this comment.
I can't help but feel a lot of this test code could be compacted in some way, as it seems quite verbose; haven't had time to properly check this however so don't really have a suggestion myself 🤔
There was a problem hiding this comment.
I'll try to make it more concise, thanks for the feedback!
There was a problem hiding this comment.
@Jefffrey I combined the implementation of eval_ansi_mode and eval_legacy_mode into one eval_with_mode macro, please take a look when you have time, thanks!
|
@hsiang-c , I would like to volunteer and contribute moving other ANSI functions from Comet back to Spark |
| self.options.execution.enforce_batch_size_in_joins | ||
| } | ||
|
|
||
| pub fn with_enable_ansi_mode(mut self, enable_ansi_mode: bool) -> Self { |
There was a problem hiding this comment.
Do we still need this function? If so it needs a docstring
There was a problem hiding this comment.
I think we need it eventually. I also refer to this config in datafusion/sqllogictest/test_files/spark/README.md now.
|
|
||
| macro_rules! eval_legacy_mode { | ||
| ($TYPE:ident, $VAL:expr) => {{ | ||
| macro_rules! eval_with_mode { |
There was a problem hiding this comment.
Is it possible to try move these tests to SLTs instead? Having so much macro code just for unit tests isn't ideal here 🤔
4 participants
Which issue does this PR close?
datafusion-sparkSpark Compatible Functions #15914Rationale for this change
absmath functionWhat changes are included in this PR?
absthrows exception on arithmetic overflow.Are these changes tested?
datafusion.execution.enable_ansi_mode = trueAre there any user-facing changes?
Yes, arithmetic overflow will be thrown when ANSI mode is on, i.e.
datafusion.execution.enable_ansi_mode=true