-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sync up supported ops for 24.02 plugin release #796
Conversation
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]> Fixes NVIDIA#795, Fixes NVIDIA#721, Fixes NVIDIA#712 This code change is to sync the supported-ops in the tools with the Plugin release 24.02.0 in addition to disabling `JsonTuple`
e2ac6f6
to
0f3be9d
Compare
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
@kuhushukla this includes fix for
|
JsonTuple,S,`json_tuple`,None,project,result,NA,NA,NA,NA,NA,NA,NA,NA,NA,NA,NA,NA,NA,NA,S,NA,NA,NA | ||
JsonToStructs,NS,`from_json`,This is disabled by default because it is currently in beta and undergoes continuous enhancements. Please consult the [compatibility documentation](../compatibility.md#json-supporting-types) to determine whether you can enable this configuration for your use case,project,jsonStr,NA,NA,NA,NA,NA,NA,NA,NA,NA,S,NA,NA,NA,NA,NA,NA,NA,NA | ||
JsonToStructs,NS,`from_json`,This is disabled by default because it is currently in beta and undergoes continuous enhancements. Please consult the [compatibility documentation](../compatibility.md#json-supporting-types) to determine whether you can enable this configuration for your use case,project,result,NA,NA,NA,NA,NA,NA,NA,NA,NA,NA,NA,NA,NA,NA,NS,PS,PS,NA | ||
JsonTuple,NS,`json_tuple`,This is disabled by default because JsonTuple on the GPU does not support all of the normalization that the CPU supports.,project,json,NA,NA,NA,NA,NA,NA,NA,NA,NA,S,NA,NA,NA,NA,NA,NA,NA,NA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattahrens FYI, the plugin release 24.02.0 has JsonTuple
enabled by default. @viadea asked to disable JsonTuple
to match the changes in RAPIDS branch-24.04.0.
if (shouldIgnore) { | ||
OpActions.IgnorePerf | ||
} else if (shouldRemove) { | ||
// shouldRemove is checked first because sometimes an exec could have both flag set to true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a quick fix for a discrepancy I found in the reporting. It had no affect on the accuracy of the results.
Some execs might have both flags set to True. In that case, "NoPerf" should take precedence.
@@ -867,7 +867,7 @@ class SQLPlanParserSuite extends BaseTestSuite { | |||
} | |||
} | |||
|
|||
test("json_tuple is supported in Generate") { | |||
ignore("json_tuple is supported in Generate: disabled as the operator is disabled by default") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disable this unit-test because the JsonTuple is not supported anymore. We do not want to remove the unit-test since this is a temporarily change.
Signed-off-by: Ahmed Hussein (amahussein) [email protected]
Fixes #795, Fixes #721, Fixes #712
This code change is to sync the supported-ops in the tools with the Plugin release 24.02.0 in addition to disabling
JsonTuple