Match GPU overwritten functions with SQL functions from FunctionRegistry #332
Conversation
|
I'm not sure this is going to do what we want. For example |
|
We discussed more on this and will change it where we find the Spark SQL function(s) for expressions that we do support because of what you brought up @revans2. We also discussed trying to annotate our functions, not unlike the registry, to be able to expose things like datatype support. When we have annotations we can then bubble them up to configs.md in a similar place. This seems to me like a different issue though, so will find a good example and file that separately. |
|
@jlowe @revans2 thanks for the suggestions, please take a look at this rendered version: https://github.com/NVIDIA/spark-rapids/blob/230edc57c084ed2f3d0ca9a6c4e470e4a5eac7fc/docs/configs.md, and see if this is more in line to what you were thinking. |
abellina
changed the title
|
I see they ended up in the anchor tag too. Let me change that. |
|
Ok, sorry about that, looks better now. |
| val sqlFunctions = sqlFunctionsByClass.get(expr.getCanonicalName) | ||
| if (sqlFunctions.isDefined) { | ||
| val nameWithSqlFunctions = s"${rule.confKey}${sqlFunctions.get.mkString(" (", ", ", ")")}" | ||
| rule.confHelp(asTable, nameWithSqlFunctions) |
There was a problem hiding this comment.
I personally don't like the sql functions with the name of the config. It feels like we are mixing concerns here. It should either be a part of the description or it should be its own separate field.
There was a problem hiding this comment.
+1, I think this is interesting enough on its own to be a separate field, SQL Function or something similar.
|
@kuhushukla sorry I am done with my changes :) Here's what it looks like now: https://github.com/NVIDIA/spark-rapids/blob/e424b117c86f9cac238a880618b3979ee3f2ddc1/docs/configs.md |
|
I assume that you didn't do this, but it looks like the tables for a lot of the other operators is messed up. The description is not being printed. |
|
@revans2 good eye, it was probably me. Sorry @kuhushukla |
|
build |
|
Ok now only the Expressions table changed. Note, I am a little worried that the rendered jekyll site will truncate it further. We may need to widen the pages there, or remove something from the expressions table (like the |
|
Removing the spark.rapids prefix makes the table much less usable. I cannot copy and paste any more. |
|
Yeah we can follow up with jekyll style updates, different pr I'd say. #340 |
…try (NVIDIA#332) * Autogenerate a list of SQL functions that are supported in the GPU * Add the sql function seq to the expression config table * Ensure tags are unchanged, only containing the confKey * Adding a SQL Functions column to the expressions table * SQL Function => SQL Function(s) * Add missing pipe
…try (NVIDIA#332) * Autogenerate a list of SQL functions that are supported in the GPU * Add the sql function seq to the expression config table * Ensure tags are unchanged, only containing the confKey * Adding a SQL Functions column to the expressions table * SQL Function => SQL Function(s) * Add missing pipe
…A#332) * initial checkin of code with job management with admin commands * make revisions to address comments in PR * Update __init__.py fix ci * Update file_transfer.py fix typo * fix ci
Signed-off-by: Jim Brennan <jimb@nvidia.com>
This change would add a dynamic table of Spark SQL functions leveraging Spark's FunctionRegistry, and then tying it to the override rules in the plugin.
A different markdown page may be nice for this too, it is roughtly 300 functions. We can pull more info from the registry also, like function signatures, but I am not sure how far we want to go. The main thing is that it would change automatically and if someone wants to find whether a SQL function has potential GPU support, it is a quick lookup.
Update: Instead of a different table, we decided to take the Expressions table we already had and augment it with the sql function from the
FunctionRegistryto begin with.