Skip to content

Conversation

@yaooqinn
Copy link
Member

@yaooqinn yaooqinn commented Oct 22, 2019

What changes were proposed in this pull request?

bit_xor(expr) - Returns the bitwise XOR of all non-null input values, or null if none

Why are the changes needed?

As we support bit_and, bit_or now, we'd better support the related aggregate function bit_xor ahead of postgreSQL, because many other popular databases support it.

http://infocenter.sybase.com/help/index.jsp?topic=/com.sybase.help.sqlanywhere.12.0.1/dbreference/bit-xor-function.html

https://dev.mysql.com/doc/refman/5.7/en/group-by-functions.html#function_bit-or

https://www.vertica.com/docs/9.2.x/HTML/Content/Authoring/SQLReferenceManual/Functions/Aggregate/BIT_XOR.htm?TocPath=SQL%20Reference%20Manual%7CSQL%20Functions%7CAggregate%20Functions%7C_____10

Does this PR introduce any user-facing change?

add a new bit agg

How was this patch tested?

UTs added

@SparkQA
Copy link

SparkQA commented Oct 22, 2019

Test build #112452 has finished for PR 26205 at commit dcd9685.

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

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

I dunno, @cloud-fan what do you think of adding this? seems niche but reasonable.

@srowen
Copy link
Member

srowen commented Oct 22, 2019

Also do you need to update docs or Pyspark with this function?

@cloud-fan
Copy link
Contributor

Also do you need to update docs or Pyspark with this function?

The newly added bitwise functions are only available in SQL.

This is a general question: shall we add scala/python/R functions for all the builtin SQL functions? I feel we only need the common ones, as users can always do expr("sql_fun(para1)") to access SQL functions in scala/python/R.

@SparkQA
Copy link

SparkQA commented Oct 23, 2019

Test build #112503 has finished for PR 26205 at commit 8e04b30.

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

@srowen
Copy link
Member

srowen commented Oct 23, 2019

Sounds fine to not add to Pyspark if other similar functions are not. I also don't see particular docs for these bitwise functions, so this may be all that's needed for consistency now.

6
""",
since = "3.0.0")
case class BitXorAgg(child: Expression) extends DeclarativeAggregate with ExpectsInputTypes {
Copy link
Member

Choose a reason for hiding this comment

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

BitAndAgg, BitOrAgg, and BitXorAgg has the similar logic, so can we share it by using a new trait (e.g., BitwiseOpLike)?

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good, will update

Copy link
Member

Choose a reason for hiding this comment

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

I think you can make it much simpler like this;

abstract class BitAggregate extends DeclarativeAggregate with ExpectsInputTypes {

  ...
  override lazy val updateExpressions: Seq[Expression] =
    If(IsNull(bitAgg),
      child,
      If(IsNull(child), bitAgg, bitOp(bitAgg, child))) :: Nil

  override lazy val mergeExpressions: Seq[Expression] =
    If(IsNull(bitAgg.left),
      bitAgg.right,
      If(IsNull(bitAgg.right), bitAgg.left, bitOp(bitAgg.left, bitAgg.right))) :: Nil
}

case class BitAndAgg(child: Expression) extends BitAggregate {

  override def nodeName: String = "bit_and"
  override def bitOp(left: Expression, right: Expression): BinaryArithmetic =
    BitwiseAnd(left, right)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

yea, much cooler, thanks

@yaooqinn
Copy link
Member Author

@srowen I guess docs will be generated via @ExpressionDescription annotation.

@SparkQA
Copy link

SparkQA commented Oct 24, 2019

Test build #112605 has finished for PR 26205 at commit ffb1a60.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • abstract class BitAggregate extends DeclarativeAggregate with ExpectsInputTypes
  • case class BitAndAgg(child: Expression) extends BitAggregate
  • case class BitOrAgg(child: Expression) extends BitAggregate
  • case class BitXorAgg(child: Expression) extends BitAggregate

@SparkQA
Copy link

SparkQA commented Oct 24, 2019

Test build #112611 has finished for PR 26205 at commit 97eeab2.

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

@maropu
Copy link
Member

maropu commented Oct 25, 2019

The tests in sql-tests/inputs/postgreSQL have been just ported from the pgSQL repo, so can you add tests in anothe place, e.g., sql-tests/inputs/bitwise.sql?

@yaooqinn
Copy link
Member Author

just for curiosity, if add same test in sql-tests/inputs/bitwise.sql, these tests will exist in three places including sql-tests/inputs/postgreSQL/udf-aggregates_part2.sql and sql-tests/inputs/postgreSQL/aggregates_part2.sql, are they too much redundant?

@maropu
Copy link
Member

maropu commented Oct 25, 2019

I meant the tests for this pr should be put only in bitwise.sql.

@yaooqinn
Copy link
Member Author

ok i got it

@SparkQA
Copy link

SparkQA commented Oct 25, 2019

Test build #112643 has finished for PR 26205 at commit 1481aa8.

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

@maropu
Copy link
Member

maropu commented Oct 25, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Oct 25, 2019

Test build #112653 has finished for PR 26205 at commit 1481aa8.

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

Copy link
Member

@maropu maropu left a comment

Choose a reason for hiding this comment

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

LGTM @cloud-fan @srowen can you recheck?

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Still looks reasonable to me.

@maropu maropu closed this in 0cf4f07 Oct 25, 2019
@maropu
Copy link
Member

maropu commented Oct 25, 2019

Thanks, @srowen. Merged to master.

@yaooqinn yaooqinn deleted the SPARK-29545 branch October 25, 2019 13:41
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.

6 participants