Skip to content

Conversation

@yaooqinn
Copy link
Member

What changes were proposed in this pull request?

bool_or(x) <=> any/some(x) <=> max(x)
bool_and(x) <=> every(x) <=> min(x)
Args:
x: boolean

Why are the changes needed?

PostgreSQL, Presto and Vertica, etc also support this feature:

Does this PR introduce any user-facing change?

add new functions support

How was this patch tested?

add ut

@SparkQA
Copy link

SparkQA commented Oct 15, 2019

Test build #112103 has finished for PR 26126 at commit 1e32603.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class BoolOr(arg: Expression) extends UnevaluableBooleanAggBase(arg)
  • case class BoolAnd(arg: Expression) extends UnevaluableBooleanAggBase(arg)

@yaooqinn
Copy link
Member Author

cc @wangyum @cloud-fan @gatorsmile

expression[EveryAgg]("every"),
expression[AnyAgg]("any"),
expression[SomeAgg]("some"),
expression[BoolAnd]("bool_and"),
Copy link
Contributor

Choose a reason for hiding this comment

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

is bool_and just an alias of every? If so we can do expression[EveryAgg]("bool_and")

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is. https://github.com/apache/spark/pull/26126/files#diff-fdccd9945d709da6c561b67a2e46a0d8R296, is there a way to keep the bool_and string in the schema?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to keep the function alias in the schema. There are many examples in FunctionRegistry, that a function has more than one name and we always use one name when displaying.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Then any, some and bool_or are mathematically equivalent, I guess we can only left AnyAgg and remove other implemtations

@cloud-fan
Copy link
Contributor

Can we make the title more explicit that this just adds function alias?

@SparkQA
Copy link

SparkQA commented Oct 16, 2019

Test build #112142 has finished for PR 26126 at commit 0a09742.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@yaooqinn yaooqinn changed the title [SPARK-27880][SQL] Add support for bool_and and bool_or aggregates [SPARK-27880][SQL] Add bool_and for every and bool_or for any as function aliases Oct 16, 2019
@yaooqinn
Copy link
Member Author

retest this please

Copy link
Member

@wangyum wangyum left a comment

Choose a reason for hiding this comment

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

We should enable these tests: aggregates_part2.sql#L116-L160 and udf-aggregates_part2.sql#L118-L162. Are we doing this in this PR or in the follow-up PR?

@SparkQA
Copy link

SparkQA commented Oct 16, 2019

Test build #112145 has finished for PR 26126 at commit 0a09742.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@yaooqinn
Copy link
Member Author

@wangyum thanks for noticing. But why these same tests in two different test files?

@SparkQA
Copy link

SparkQA commented Oct 16, 2019

Test build #112160 has finished for PR 26126 at commit ce32134.

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

@SparkQA
Copy link

SparkQA commented Oct 16, 2019

Test build #112162 has finished for PR 26126 at commit 547c524.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 6d4cc7b Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants