[SPARK-31476][SQL][FOLLOWUP] Add tests for extract('field', source) #28276
Conversation
| select extract(not_supported from i) from t; | ||
|
|
||
| -- In SPARK-31476, we've supported extract('field', source), too | ||
| select date_part('millennium', c) from t; |
There was a problem hiding this comment.
I added only simple test patterns for extract('field', source).
There was a problem hiding this comment.
I think only a couple of tests are fine :-).
There was a problem hiding this comment.
BTW, why is it date_part instead of extract? date_part seems being tested at date_part.sql.
There was a problem hiding this comment.
Wait .. are DatePart and Extract the same? If so, it might be better to have one class and handle aliases via expression(..., setAlias=true) at FunctionRegistry.
There was a problem hiding this comment.
Actually, this support (extract('field', source)) is a side-effect of #31476.... EXTRACT should be formed as EXTRACT(field FROM source), so the example field in ExpressionDescription is different now from each other. Related discussion: #21479 (comment) cc: @cloud-fan
There was a problem hiding this comment.
I see. +1 to have a separate examples and documentation.
There was a problem hiding this comment.
I think the side effect is we support extract('field', source), why we are testing date_part here...
There was a problem hiding this comment.
Ur, my bad... I'll update soon...
|
Test build #121551 has finished for PR 28276 at commit
|
|
Test build #121577 has finished for PR 28276 at commit
|
|
Although this comes first, shall we wait for #28284 ? That will remove some support fields. So, it will reduce the number of test cases here. |
|
sure, thanks, @dongjoon-hyun |
|
#28284 has been merged, let's update the tests here |
maropu
force-pushed
the
SPARK-31476-FOLLOWUP
branch
from
cf9a9ea to
04642b3
Compare
|
ok, udpated. |
| struct<> | ||
| -- !query output | ||
| org.apache.spark.sql.AnalysisException | ||
| Literals of type 'millennium' are currently not supported for the string type.;; line 1 pos 7 |
There was a problem hiding this comment.
let's remove these unsupported tests
maropu
force-pushed
the
SPARK-31476-FOLLOWUP
branch
from
f40578d to
28758ae
Compare
|
Test build #121619 has finished for PR 28276 at commit
|
|
Test build #121631 has finished for PR 28276 at commit
|
|
thanks, merging to master/3.0! |
### What changes were proposed in this pull request? SPARK-31476 has supported `extract('field', source)` as side-effect, so this PR intends to add some tests for the function in `SQLQueryTestSuite`. ### Why are the changes needed? For better test coverage. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Added tests. Closes #28276 from maropu/SPARK-31476-FOLLOWUP. Authored-by: Takeshi Yamamuro <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 820733a) Signed-off-by: Wenchen Fan <[email protected]>
|
+1, late LGTM. Thank you all! |
5 participants
What changes were proposed in this pull request?
SPARK-31476 has supported
extract('field', source)as side-effect, so this PR intends to add some tests for the function inSQLQueryTestSuite.Why are the changes needed?
For better test coverage.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added tests.