fix: panic and incorrect results in `LogFunc::output_ordering()` #11571

jonahgao · 2024-07-20T14:44:24Z

Which issue does this PR close?

Closes #11549.

Rationale for this change

The current implementation assumes log has two parameters, but this does not hold for log(100), which is equivalent to log(10, 100). This will cause panic due to index out of bounds.
It mistakenly treats the second parameter as base, causing incorrect ordering results.

Ref: https://datafusion.apache.org/user-guide/sql/scalar_functions.html#log

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

No

jonahgao · 2024-07-20T14:45:56Z

datafusion/sqllogictest/test_files/order.slt

@@ -547,32 +554,32 @@ physical_plan
 04)------CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100.csv]]}, projection=[c11], output_ordering=[c11@0 ASC NULLS LAST], has_header=true

 query TT
-  EXPLAIN SELECT LOG(c11, c12) as log_c11_base_c12
+  EXPLAIN SELECT LOG(c12, c11) as log_c11_base_c12


base should be placed in the first parameter.

2010YOUY01

Thank you for this rapid fix!

It's not the problem for this PR, but I found this output_ordering calculation is extremely hard to get all cases correct, I think the correct way to implement it is to do some hard coded exact match only for a small number of obvious cases. For the rest cases we could blindly return unordered.

If the example I gave is not mistaken, we can open an issue to address it and fix later.

2010YOUY01 · 2024-07-21T10:07:54Z

datafusion/functions/src/math/log.rs

-        match (input[0].sort_properties, input[1].sort_properties) {
+        let (base_sort_properties, num_sort_properties) = if input.len() == 1 {
+            // log(x) defaults to log(10, x)
+            (SortProperties::Singleton, input[0].sort_properties)


This change makes sense to me 👍🏼

2010YOUY01 · 2024-07-21T10:12:56Z

datafusion/functions/src/math/log.rs

+        } else {
+            (input[0].sort_properties, input[1].sort_properties)
+        };
+        match (num_sort_properties, base_sort_properties) {


Looks like the following logic is not completely correct 🤔
e.g.
base is DESC NULLS FIRST
num is ASC NULLS LAST
This implementation will return log(base, num) is ASC NULLS LAST
But the actual output might look like

NULL NULL 1 2 NULL

Which is unordered.

I agree with you. I noticed that other logic also does not consider nulls_first, for example

datafusion/datafusion/expr/src/sort_properties.rs

Lines 48 to 62 in 36660fe

pub fn add(&self, rhs: &Self) -> Self {

match (self, rhs) {

(Self::Singleton, _) => *rhs,

(_, Self::Singleton) => *self,

(Self::Ordered(lhs), Self::Ordered(rhs))

if lhs.descending == rhs.descending =>

{

Self::Ordered(SortOptions {

descending: lhs.descending,

nulls_first: lhs.nulls_first || rhs.nulls_first,

})

}

_ => Self::Unordered,

}

}

Maybe @berkaysynnada can help confirm it.

You are correct. We also need to check the placement of nulls in output_ordering() (and possibly in other implementations of it for multi-input functions). As @jonahgao noticed, impl SortProperties {} needs similar handling as well, but that can be addressed in a separate PR. I can create a ticket for it.

Fixed the nulls_first problem for log by 3feeaa8. We can create separate PRs for other fixes.

cc @2010YOUY01 @berkaysynnada

berkaysynnada

The fix looks good to me. Thanks, @jonahgao. Could you also address the missing cases of nulls order checks, or do we report a ticket?

berkaysynnada · 2024-07-22T06:57:36Z

datafusion/functions/src/math/log.rs

-        match (input[0].sort_properties, input[1].sort_properties) {
+        let (base_sort_properties, num_sort_properties) = if input.len() == 1 {
+            // log(x) defaults to log(10, x)
+            (SortProperties::Singleton, input[0].sort_properties)


berkaysynnada · 2024-07-22T07:06:34Z

datafusion/functions/src/math/log.rs

+        } else {
+            (input[0].sort_properties, input[1].sort_properties)
+        };
+        match (num_sort_properties, base_sort_properties) {


You are correct. We also need to check the placement of nulls in output_ordering() (and possibly in other implementations of it for multi-input functions). As @jonahgao noticed, impl SortProperties {} needs similar handling as well, but that can be addressed in a separate PR. I can create a ticket for it.

berkaysynnada · 2024-07-22T07:09:00Z

datafusion/functions/src/math/log.rs

+            SortProperties::Singleton,
+        ];
+        assert_eq!(results, expected);
+    }


Thanks for adding these tests. output_ordering() API of scalar functions are experimental and needs unit tests (#10595). This is a good example for the rest of them.

output_ordering() API of scalar functions are experimental

What do you mean by "experimental"? If that is the case perhaps we can update the documentation to explain what that means -- I don't see any mention of it

https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.ScalarUDFImpl.html#method.output_ordering

jonahgao · 2024-07-22T08:35:46Z

datafusion/sqllogictest/test_files/order.slt

@@ -512,7 +519,7 @@ CREATE EXTERNAL TABLE aggregate_test_100 (
 )
 STORED AS CSV
 WITH ORDER(c11)
-WITH ORDER(c12 DESC)
+WITH ORDER(c12 DESC NULLS LAST)


To make c11 and c12 have the same nulls_first.

jonahgao · 2024-07-22T14:14:16Z

The fix looks good to me. Thanks, @jonahgao. Could you also address the missing cases of nulls order checks, or do we report a ticket?

Filed #11596

alamb

Thank you @jonahgao @berkaysynnada and @2010YOUY01 -- I think this PR is an improvement over what is on main,

As you have pointed out, filing some tickets to track the remaining work would be a good idea.

alamb · 2024-07-23T21:59:09Z

datafusion/functions/src/math/log.rs

+            SortProperties::Singleton,
+        ];
+        assert_eq!(results, expected);
+    }


output_ordering() API of scalar functions are experimental

What do you mean by "experimental"? If that is the case perhaps we can update the documentation to explain what that means -- I don't see any mention of it

https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.ScalarUDFImpl.html#method.output_ordering

jonahgao · 2024-07-24T01:47:20Z

Thanks all. The remaining issues are being tracked in ticket #11596

berkaysynnada · 2024-07-24T06:52:09Z

What do you mean by "experimental"

@alamb I guess I used a wrong terminology. I meant that output_ordering() methods of ScalarUDF's are not well-tested by slt or unit tests.

fix: panic and incorrect results in LogFunc::output_ordering()

22552df

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jul 20, 2024

jonahgao commented Jul 20, 2024

View reviewed changes

2010YOUY01 approved these changes Jul 21, 2024

View reviewed changes

berkaysynnada reviewed Jul 22, 2024

View reviewed changes

fix for nulls_first

3feeaa8

jonahgao commented Jul 22, 2024

View reviewed changes

jonahgao mentioned this pull request Jul 22, 2024

Propagation of ordered SortProperties should consider nulls_first #11596

Open

alamb approved these changes Jul 23, 2024

View reviewed changes

jonahgao merged commit c8ef545 into apache:main Jul 24, 2024
24 checks passed

jonahgao deleted the log_panic branch July 24, 2024 01:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: panic and incorrect results in `LogFunc::output_ordering()` #11571

fix: panic and incorrect results in `LogFunc::output_ordering()` #11571

jonahgao commented Jul 20, 2024

jonahgao Jul 20, 2024

2010YOUY01 left a comment

2010YOUY01 Jul 21, 2024

berkaysynnada Jul 22, 2024

2010YOUY01 Jul 21, 2024

jonahgao Jul 21, 2024 •

edited

Loading

berkaysynnada Jul 22, 2024

jonahgao Jul 22, 2024

berkaysynnada left a comment

berkaysynnada Jul 22, 2024

berkaysynnada Jul 22, 2024

berkaysynnada Jul 22, 2024

alamb Jul 23, 2024

jonahgao Jul 22, 2024

jonahgao commented Jul 22, 2024

alamb left a comment

alamb Jul 23, 2024

jonahgao commented Jul 24, 2024

berkaysynnada commented Jul 24, 2024

	pub fn add(&self, rhs: &Self) -> Self {
	match (self, rhs) {
	(Self::Singleton, _) => *rhs,
	(_, Self::Singleton) => *self,
	(Self::Ordered(lhs), Self::Ordered(rhs))
	if lhs.descending == rhs.descending =>
	{
	Self::Ordered(SortOptions {
	descending: lhs.descending,
	nulls_first: lhs.nulls_first \|\| rhs.nulls_first,
	})
	}
	_ => Self::Unordered,
	}
	}

fix: panic and incorrect results in LogFunc::output_ordering() #11571

fix: panic and incorrect results in LogFunc::output_ordering() #11571

Conversation

jonahgao commented Jul 20, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

2010YOUY01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgao Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkaysynnada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgao commented Jul 22, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgao commented Jul 24, 2024

berkaysynnada commented Jul 24, 2024

fix: panic and incorrect results in `LogFunc::output_ordering()` #11571

fix: panic and incorrect results in `LogFunc::output_ordering()` #11571

jonahgao Jul 21, 2024 •

edited

Loading