More advanced type checking and documentation #1323
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
revans2
requested review from
GaryShen2008,
jlowe,
NvTimLiu and
tgravescs
as code owners
Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
revans2
force-pushed
the
type_doc_check_support
branch
from
752c139
to
23a860a
Compare
|
build |
|
build |
|
This looks OK to me. Would like to see a bit more documentation for the TypeChecks functions, but that's minor. |
|
@jlowe I upmerged and updated the scaladocs for the new type checking. |
|
build |
jlowe
reviewed
Dec 14, 2020
sql-plugin/src/main/scala/com/nvidia/spark/rapids/TypeChecks.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/TypeChecks.scala
Outdated
Show resolved
Hide resolved
|
build |
jlowe
previously approved these changes
Dec 14, 2020
|
build |
|
could you add a high level overview of how this is supposed to work in description. |
tgravescs
reviewed
Dec 15, 2020
| } | ||
|
|
||
| /** | ||
| * N/A neither spark not the plugin supports this. |
| * Both Spark and the plugin support this. | ||
| * @param asterisks true if we need to include an asterisks because it is not really 100% supported. | ||
| */ | ||
| class Supported(val asterisks: Boolean = false) extends SupportLevel { |
There was a problem hiding this comment.
do we need note here for asterisks or do you end up seeing that elsewhere?
tgravescs
reviewed
Dec 15, 2020
| * A signature for types that are generally supported by the plugin. Please make sure to check | ||
| * what Spark actually supports instead of blindly using this in a signature. | ||
| */ | ||
| val legacySupportedTypes: TypeSig = BOOLEAN + BYTE + SHORT + INT + LONG + FLOAT + DOUBLE + DATE + |
There was a problem hiding this comment.
legacy makes me think this is old deprecated types (it might just be me though), would it be better to say something like basic types or commonSupportedTypes. Not a big deal though as I can't think of anything great either.
|
build |
|
@tgravescs and @jlowe could you take another look? |
jlowe
approved these changes
Dec 15, 2020
tgravescs
approved these changes
Dec 15, 2020
nartal1
pushed a commit
to nartal1/spark-rapids
that referenced
this pull request
Jun 9, 2021
Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
nartal1
pushed a commit
to nartal1/spark-rapids
that referenced
this pull request
Jun 9, 2021
Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
tgravescs
pushed a commit
to tgravescs/spark-rapids
that referenced
this pull request
Nov 30, 2023
…IDIA#1323) Signed-off-by: spark-rapids automation <70000568+nvauto@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I know this is a huge change, and I am happy to put in some comments in the code if needed. Please take a look.