[SPARK-50055][SQL] Add TryMakeInterval alternative #48580
Conversation
| ceiling.__doc__ = pysparkfuncs.ceiling.__doc__ | ||
|
|
||
|
|
||
| def try_conv(col: "ColumnOrName", fromBase: int, toBase: int) -> Column: |
There was a problem hiding this comment.
Let's add a test at test_functions.py so the tests can be shared between Spark Classic and Spark Connect.
mihailomilosevic2001
changed the title
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
Don't think it is good idea to mix two new expressions in one PR especially when they are unrelated. I believe it is better to open separate PRs per expressions.
mihailomilosevic2001
changed the title
…ryConv-TryMakeInterval # Conflicts: # docs/sql-ref-ansi-compliance.md
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
@mihailom-db Could you resolve conflicts and fix the test, it seems it is related to your changes:
ERROR [0.217s]: test_try_make_interval (pyspark.sql.tests.test_functions.FunctionsTests.test_try_make_interval)
| years: Optional["ColumnOrName"] = None, | ||
| months: Optional["ColumnOrName"] = None, | ||
| weeks: Optional["ColumnOrName"] = None, | ||
| days: Optional["ColumnOrName"] = None, | ||
| hours: Optional["ColumnOrName"] = None, | ||
| mins: Optional["ColumnOrName"] = None, | ||
| secs: Optional["ColumnOrName"] = None, |
| years: Optional["ColumnOrName"] = None, | ||
| months: Optional["ColumnOrName"] = None, | ||
| weeks: Optional["ColumnOrName"] = None, | ||
| days: Optional["ColumnOrName"] = None, | ||
| hours: Optional["ColumnOrName"] = None, | ||
| mins: Optional["ColumnOrName"] = None, | ||
| secs: Optional["ColumnOrName"] = None, |
| |100 years | | ||
| +-----------------------------------------+ | ||
|
|
||
| Example 8: Try make interval. |
There was a problem hiding this comment.
Could you add an example which shows the difference between try and non-try versions.
| * @group url_funcs | ||
| * @since 4.0.0 | ||
| */ | ||
| def try_make_interval(): Column = |
There was a problem hiding this comment.
Why do you need this one? In which cases does make_interval() fail?
There was a problem hiding this comment.
You are right, we cannot get an error here, as this is the same as makeinterval(0) which is not an overflow. Will update the pr.
…eInterval # Conflicts: # docs/sql-ref-ansi-compliance.md # python/docs/source/reference/pyspark.sql/functions.rst # sql/core/src/test/resources/sql-functions/sql-expression-schema.md
|
@MaxGekk @HyukjinKwon Do we want to not support try_make_interval in pyspark? This PR seems to block it #46975. I believe we should update that make_interval is not supported as well, as CalendarIntervalType is not supported. |
|
It's fine to have that expression. They can still perform computation with the type. It's just that the ser/de to Python isn't supported. |
|
Should I just then remove it from python API? As we cannot really call it there. |
|
hm, we can call them, no? It's just that we don't support |
|
e.g., you can call |
|
Aha, ok, so I should only change the test to use some other type of collection of data that is not |
|
Yeah, i think so. |
|
Ready for merge now @MaxGekk @HyukjinKwon |
|
+1, LGTM. Merging to master. |
3 participants
What changes were proposed in this pull request?
Addition of two new expression try_make_interval.
Why are the changes needed?
This is a split PR from #48499 so that we divide the reasonings for PRs.
Does this PR introduce any user-facing change?
Yes, new expressions added.
How was this patch tested?
Tests added.
Was this patch authored or co-authored using generative AI tooling?
No.