Skip to content

[SPARK-30048][SQL] Enable aggregates with interval type values for RelationalGroupedDataset - #26681

Closed
yaooqinn wants to merge 6 commits into
apache:masterfrom
yaooqinn:SPARK-30048
Closed

[SPARK-30048][SQL] Enable aggregates with interval type values for RelationalGroupedDataset#26681
yaooqinn wants to merge 6 commits into
apache:masterfrom
yaooqinn:SPARK-30048

Conversation

@yaooqinn

@yaooqinnyaooqinn commented Nov 26, 2019

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Now the min/max/sum/avg are support for intervals, we should also enable it in RelationalGroupedDataset

Why are the changes needed?

API consistency improvement

Does this PR introduce any user-facing change?

yes, Dataset support min/max/sum/avg(mean) on intervals

How was this patch tested?

add ut

@yaooqinn

yaooqinn commented Nov 26, 2019

Copy link
Copy Markdown
MemberAuthor

cc @cloud-fan@maropu@HyukjinKwon@wangyum, thanks for reviewing.

maropu
maropu reviewed Nov 26, 2019
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/RelationalGroupedDataset.scala
colNames.map { colName =>
val namedExpr = df.resolve(colName)
if (!namedExpr.dataType.isInstanceOf[NumericType]) {
if (!TypeCollection.NumericAndInterval.acceptsType(namedExpr.dataType)) {

@maropumaropuNov 26, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the comment to make it more general one?

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/types/AbstractDataType.scala

Line 82 in c2d513f

* Types that include numeric types and interval type. They are only used in unary_minus,

@yaooqinnyaooqinnNov 26, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your suggestion. Please check 。

cloud-fan
cloud-fan reviewed Nov 26, 2019
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/Dataset.scala Outdated
schema.fields.filter{ f =>
TypeCollection.NumericAndInterval.acceptsType(f.dataType)
}.map { n =>
queryExecution.analyzed.resolveQuoted(n.name, sparkSession.sessionState.analyzer.resolver).get

@cloud-fancloud-fanNov 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can do

queryExecution.analyzed.output.filter { attr =>
TypeCollection.NumericAndInterval.acceptsType(attr.dataType)
}

@SparkQA

SparkQA commented Nov 26, 2019

Copy link
Copy Markdown

Test build #114462 has finished for PR 26681 at commit 4b353df.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Nov 26, 2019

Copy link
Copy Markdown

Test build #114469 has finished for PR 26681 at commit f93cd9f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Nov 26, 2019

Copy link
Copy Markdown

Test build #114472 has finished for PR 26681 at commit 73b217a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Nov 27, 2019

Copy link
Copy Markdown

Test build #114499 has finished for PR 26681 at commit fe84ae8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Nov 27, 2019

Copy link
Copy Markdown

Test build #114519 has finished for PR 26681 at commit 69667c2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • public class GetLocalDirsForExecutors extends BlockTransferMessage
  • public class LocalDirsForExecutors extends BlockTransferMessage
  • sealed trait ViewType
  • case class CreateViewStatement(

@SparkQA

SparkQA commented Dec 2, 2019

Copy link
Copy Markdown

Test build #114731 has finished for PR 26681 at commit 6dca7b1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • trait OperationHelper
  • class HiveThriftServer2AppStatusStore(
  • class HiveThriftServer2HistoryServerPlugin extends AppHistoryServerPlugin

@maropu

maropu commented Dec 2, 2019

Copy link
Copy Markdown
Member

cc: @cloud-fan@dongjoon-hyun

@cloud-fan

cloud-fan commented Dec 3, 2019

Copy link
Copy Markdown
Contributor

thanks, merging to master!

@yaooqinn
yaooqinn deleted the SPARK-30048 branch December 3, 2019 11:35
attilapiros pushed a commit to attilapiros/spark that referenced this pull request Dec 6, 2019
@yaooqinn@attilapiros
[SPARK-30048][SQL] Enable aggregates with interval type values for Re…
14a348f
…lationalGroupedDataset
### What changes were proposed in this pull request?
Now the min/max/sum/avg are support for intervals, we should also enable it in RelationalGroupedDataset
### Why are the changes needed?
API consistency improvement
### Does this PR introduce any user-facing change?
yes, Dataset support min/max/sum/avg(mean) on intervals
### How was this patch tested?
add ut
Closesapache#26681 from yaooqinn/SPARK-30048.
Authored-by: Kent Yao <yaooqinn@hotmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@cloud-fancloud-fan mentioned this pull request Jan 17, 2020
Closed
cloud-fan pushed a commit that referenced this pull request Jan 19, 2020
@yaooqinn@cloud-fan
[SPARK-30551][SQL] Disable comparison for interval type
17857f9
### What changes were proposed in this pull request?
As we are not going to follow ANSI to implement year-month and day-time interval types, it is weird to compare the year-month part to the day-time part for our current implementation of interval type now.
Additionally, the current ordering logic comes from PostgreSQL where the implementation of the interval is messy. And we are not aiming PostgreSQL compliance at all.
THIS PR will revert #26681 and #26337
### Why are the changes needed?
make interval type more future-proofing
### Does this PR introduce any user-facing change?
there are new in 3.0, so no
### How was this patch tested?
existing uts shall work
Closes#27262 from yaooqinn/SPARK-30551.
Authored-by: Kent Yao <yaooqinn@hotmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan left review comments
@maropumaropumaropu approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yaooqinn@SparkQA@maropu@cloud-fan@dongjoon-hyun