[native] Handle NULLIF without the need for specialForm by vermapratyush · Pull Request #19791 · prestodb/presto

vermapratyush · 2023-06-05T11:19:50Z

The usage of type coercion in NULLIF in presto/java made it incompatible with
presto native, this was made possible by specialForm. Handle NULLIF in presto
native using the expanded form:

if (equal(cast(first as ), cast(second as )), cast(null as firstType), first)

Extend TestExpressionCompiler to native test suite with addition of nullIf in
GeneralQueries test suite.

== NO RELEASE NOTE ==

rschlussel

can't find the actual change. can you point me to which file it's in? also, can you add an end to end test to AbstractTestQueries to make sure a full query will work (I think presto native extends/runs all those queries, right? if not, then some equivalent to add a test for both java and for native implementations).

rschlussel · 2023-06-26T15:52:17Z

...execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestExpressionCompiler.java

Are these all things that need to be fixed for native? Should we create an issue and link it in the comment here?

Created an issue and added it to comment. #19999

@majetideepak Deepak, is this something you could help look into?

@mbasmanova sure.

rschlussel · 2023-06-26T15:52:51Z

...execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestExpressionCompiler.java

can you clarify this? Shouldn't they still pass even if they don't have custom handling?

They still pass, but running them seems redundant as there is no custom handling. The queries gets executed within JVM (constant folding) and does not touch the cpp server.

presto-main/src/main/java/com/facebook/presto/sql/relational/SqlToRowExpressionTranslator.java

rschlussel

Never mind about not being able to find the main change. Looks like it was a temporary state while you were updating the branch.

presto-main/src/test/java/com/facebook/presto/sql/gen/TestExpressionCompiler.java

...ution/src/test/java/com/facebook/presto/nativeworker/TestPrestoNativeGeneralQueriesJSON.java

rschlussel · 2023-06-26T17:38:14Z

also fix checkstyle violations

vermapratyush · 2023-06-27T13:02:41Z

The test failure is unrelated to this change, I am looking at fixing the test here. #20003

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

rschlussel · 2023-06-27T13:43:21Z

presto-main/src/main/java/com/facebook/presto/sql/relational/SqlFunctionUtils.java

                        Optional.empty(),
                        sqlFunctionProperties,
                        sessionFunctions,
+                        false,


is this safe to assume it's not native?

I don't see this code path being triggered in native path.
It is used in

https://github.com/vermapratyush/presto/blob/d5eed561c31e21cb0b6cfa5f94038d9d4f2b4b89/presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineSqlFunctions.java#L106
https://github.com/prestodb/presto/blob/d5eed561c31e21cb0b6cfa5f94038d9d4f2b4b89/presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineSqlFunctions.java#L106 - should not be invoked in native flow

https://github.com/vermapratyush/presto/blob/d5eed561c31e21cb0b6cfa5f94038d9d4f2b4b89/presto-main/src/main/java/com/facebook/presto/sql/gen/RowExpressionCompiler.java#L144
https://github.com/prestodb/presto/blob/d5eed561c31e21cb0b6cfa5f94038d9d4f2b4b89/presto-main/src/main/java/com/facebook/presto/sql/gen/RowExpressionCompiler.java#L144 - should not be invoked in native flow

https://github.com/vermapratyush/presto/blob/d5eed561c31e21cb0b6cfa5f94038d9d4f2b4b89/presto-main/src/main/java/com/facebook/presto/sql/planner/RowExpressionInterpreter.java#L282
https://github.com/prestodb/presto/blob/d5eed561c31e21cb0b6cfa5f94038d9d4f2b4b89/presto-main/src/main/java/com/facebook/presto/sql/planner/RowExpressionInterpreter.java#L282 - Not sure about this one (i don’t see this getting invoked from the e2e unit tests)
Do you know if RowExpressionInterpreter is of concern?

I think these can be invoked from native flow if someone uses a sql function that uses NULLIF in its implementation. And seems like an easy place for bugs to lurk.

Could this be because all the native tests set configuration "inline-sql-functions" to false ? https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/NativeQueryRunnerUtils.java#L32

Ideally, we should try to avoid such properties/configurations.

But is it possible to use that property instead in this rule ?

@aditi-pandit Can you please elaborate what do you mean by can we use that property instead of this rule?

@vermapratyush : I'm not super familiar with optimizer code, but it is possible that instead of hardcoding to false, we get the value for inline-sql-functions property at a higher level (or here itself if there is access to it) and use that instead in this code ?

The issue with inline-sql-functions is the same as that of using Session to determine if the query is for native, certain code-flow does not have session object.
I also verified with the velox team, and it would not make sense to provide support for SQL function for native queries within the java stack. It is better to support sql function by implementing it natively in velox itself. So I think hard-coding is ok here.

...e-execution/src/test/java/com/facebook/presto/spark/TestPrestoSparkNativeGeneralQueries.java

rschlussel

Summarizing discussion:
This fixes handling of NULLIF for native for all code paths that native queries currently go through. The SqlFunctionUtils, which are used in the sql function code path does not currently properly support native, since there is no access to the session there, only connector session. Sqlfunctions are disabled in native, so this path isn't currently accessed, but it's a place for surprise bugs for the future. This is still a todo to fix, but we don't want to block the main issue on addressing this case: #20008.

mbasmanova · 2023-06-29T19:38:51Z

presto-main/src/main/java/com/facebook/presto/sql/relational/SqlFunctionUtils.java

Why can't we address this TODO now?

Detailed conversation in this thread: https://github.com/prestodb/presto/pull/19791/files/189afe0a571d7ee128f242fd803a6098d967861b#r1243767843

In short,

The SqlFunctionUtils, which are used in the sql function code path does not currently properly support native, since there is no access to the session there, only connector session.

the solution would be to add the isNative flag to the connector session and pass it through when we create the connector session. I think that's a very reasonable to do, and there may be other places that would want to handle things differently depending on the native flag too, so could be generally useful.

mbasmanova

@vermapratyush There was custom translation of NULLIF expression in Prestissimo. I assume we do not need this anymore. Let's remove.

mbasmanova

@rschlussel @vermapratyush Do you have a sense of any performance penalty this implementation of NULLIF would have?

rschlussel · 2023-06-29T20:42:57Z

@rschlussel @vermapratyush Do you have a sense of any performance penalty this implementation of NULLIF would have?

it's doing the same translation that the custom cpp code is doing, so should be the same. Only overhead would be if it's adding casts that the cpp function doesn't need (e.g. type only coercion)

mbasmanova · 2023-06-29T21:02:27Z

...e-execution/src/test/java/com/facebook/presto/spark/TestPrestoSparkNativeGeneralQueries.java

These tests look very similar to me. What's the motivation for adding so many nearly identical tests?

They are permutation combination of bigint, double and empty rows (see date filter).

...e-execution/src/test/java/com/facebook/presto/spark/TestPrestoSparkNativeGeneralQueries.java

The usage of type coercion in NULLIF in presto/java made it incompatible with presto native, this was made possible by specialForm. Handle NULLIF in presto native using the expanded form: > if (equal(cast(first as <common type>), cast(second as <common type>)), cast( null as firstType), first) Extend TestExpressionCompiler to native test suite with addition of nullIf in GeneralQueries test suite.

feilong-liu self-requested a review June 5, 2023 21:35

vermapratyush closed this Jun 14, 2023

vermapratyush reopened this Jun 15, 2023

vermapratyush force-pushed the null-if branch from 809e13f to fee8847 Compare June 22, 2023 12:14

vermapratyush changed the title ~~[native] Add additional cast to NULL_IF is argument types are different~~ [native] Handle NULLIF without the need for specialForm Jun 22, 2023

vermapratyush requested a review from rschlussel June 22, 2023 12:15

vermapratyush force-pushed the null-if branch from fee8847 to b79d232 Compare June 22, 2023 12:16

vermapratyush marked this pull request as ready for review June 22, 2023 12:16

vermapratyush requested review from a team as code owners June 22, 2023 12:16

vermapratyush requested a review from presto-oss June 22, 2023 12:16

vermapratyush force-pushed the null-if branch 7 times, most recently from 12191ca to 997f175 Compare June 26, 2023 15:50

rschlussel reviewed Jun 26, 2023

View reviewed changes

vermapratyush force-pushed the null-if branch from b65ba5f to b7661c6 Compare June 26, 2023 16:02

rschlussel reviewed Jun 26, 2023

View reviewed changes

presto-main/src/test/java/com/facebook/presto/sql/gen/TestExpressionCompiler.java Outdated Show resolved Hide resolved

...ution/src/test/java/com/facebook/presto/nativeworker/TestPrestoNativeGeneralQueriesJSON.java Outdated Show resolved Hide resolved

vermapratyush force-pushed the null-if branch 3 times, most recently from 5b1e24b to b61e779 Compare June 27, 2023 12:24

vermapratyush requested a review from rschlussel June 27, 2023 13:02

rschlussel reviewed Jun 27, 2023

View reviewed changes

vermapratyush force-pushed the null-if branch from b61e779 to 871c53c Compare June 27, 2023 16:23

[refactor] Pass functionAndTypeManager to SqlToRowExpressionTranslator

0e68b49

vermapratyush force-pushed the null-if branch 3 times, most recently from 5ed517c to c15e61a Compare June 29, 2023 10:01

vermapratyush requested a review from rschlussel June 29, 2023 10:04

vermapratyush force-pushed the null-if branch from c15e61a to 189afe0 Compare June 29, 2023 10:29

rschlussel approved these changes Jun 29, 2023

View reviewed changes

mbasmanova reviewed Jun 29, 2023

View reviewed changes

mbasmanova approved these changes Jun 29, 2023

View reviewed changes

vermapratyush force-pushed the null-if branch from 189afe0 to b2ab4a9 Compare June 29, 2023 20:55

mbasmanova reviewed Jun 29, 2023

View reviewed changes

...e-execution/src/test/java/com/facebook/presto/spark/TestPrestoSparkNativeGeneralQueries.java Outdated Show resolved Hide resolved

vermapratyush force-pushed the null-if branch from b2ab4a9 to 565bb22 Compare June 29, 2023 21:29

vermapratyush mentioned this pull request Jun 29, 2023

[native] Revert NULLIF specialForm support #20031

Merged

vermapratyush merged commit e291329 into prestodb:master Jun 29, 2023

vermapratyush deleted the null-if branch June 29, 2023 22:26

kewang1024 mentioned this pull request Jul 1, 2023

[native] Advance velox and disable nullif tests. #20037

Merged

vermapratyush mentioned this pull request Jul 2, 2023

[native] Handle NULLIF in prestissimo #20040

Closed

Conversation

vermapratyush commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rschlussel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vermapratyush Jun 27, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rschlussel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rschlussel commented Jun 26, 2023

Uh oh!

vermapratyush commented Jun 27, 2023

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rschlussel 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!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

rschlussel commented Jun 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vermapratyush Jun 29, 2023 • 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vermapratyush commented Jun 5, 2023 •

edited

Loading

vermapratyush Jun 27, 2023 •

edited

Loading

vermapratyush Jun 29, 2023 •

edited

Loading