fix: panic and incorrect results in `LogFunc::output_ordering()` by jonahgao · Pull Request #11571 · apache/datafusion

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


 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

-        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

+        } 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

-        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

+        } 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

+            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

 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

+            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

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

	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,
	}
	}

Uh oh!

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2010YOUY01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahgao Jul 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkaysynnada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahgao commented Jul 22, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahgao commented Jul 24, 2024

Uh oh!

berkaysynnada commented Jul 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonahgao Jul 21, 2024 •

edited

Loading