Skip to content

[SPARK-23918][SQL] Add array_min function #21025

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
mgaido91 wants to merge 9 commits into from
Closed

[SPARK-23918][SQL] Add array_min function #21025

mgaido91 wants to merge 9 commits into from

Conversation

@mgaido91
Copy link
Contributor

@mgaido91 mgaido91 commented Apr 10, 2018

What changes were proposed in this pull request?

The PR adds the SQL function array_min. It takes an array as argument and returns the minimum value in it.

How was this patch tested?

added UTs

HyukjinKwon
HyukjinKwon reviewed Apr 10, 2018
View reviewed changes
python/pyspark/sql/functions.py Outdated
>>> df = spark.createDataFrame([([2, 1, 3],), ([None, 10, -1],)], ['data'])
>>> df.select(array_min(df.data).alias('min')).collect()
[Row(min=1), Row(min=-1)]
"""
Copy link
Member

@HyukjinKwon HyukjinKwon Apr 10, 2018

Choose a reason for hiding this comment

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

quick nit

Copy link
Contributor Author

@mgaido91 mgaido91 Apr 10, 2018

Choose a reason for hiding this comment

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

sorry, I can't see what is the problem here. May you please clarify? Thanks.

Copy link
Member

@HyukjinKwon HyukjinKwon Apr 10, 2018

Choose a reason for hiding this comment

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

""" seems having one more leading space ..

Copy link
Contributor Author

@mgaido91 mgaido91 Apr 10, 2018

Choose a reason for hiding this comment

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

you are right, good catch! I was looking for reference at the sort_array function below which has the same issue. I will fix it there too, thanks.

HyukjinKwon
HyukjinKwon reviewed Apr 10, 2018
View reviewed changes
...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
Examples:
> SELECT _FUNC_(array(1, 20, null, 3));
1
""", since = "2.4.0")
Copy link
Member

@HyukjinKwon HyukjinKwon Apr 10, 2018

Choose a reason for hiding this comment

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

indentation ..

@SparkQA
Copy link

SparkQA commented Apr 10, 2018

Test build #89113 has finished for PR 21025 at commit b176f8d.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class ArrayMin(child: Expression) extends UnaryExpression with ImplicitCastInputTypes

@gatorsmile
Copy link
Member

gatorsmile commented Apr 10, 2018

cc @ueshin

@SparkQA
Copy link

SparkQA commented Apr 10, 2018

Test build #89121 has finished for PR 21025 at commit 626f8cd.

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

@SparkQA
Copy link

SparkQA commented Apr 10, 2018

Test build #89125 has finished for PR 21025 at commit fbb9dc1.

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

@kiszk
Copy link
Member

kiszk commented Apr 10, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Apr 10, 2018

Test build #89140 has finished for PR 21025 at commit fbb9dc1.

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

mn-mikke
mn-mikke reviewed Apr 11, 2018
View reviewed changes
...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala Outdated
override def nullable: Boolean =
child.nullable || child.dataType.asInstanceOf[ArrayType].containsNull

override def foldable: Boolean = child.foldable
Copy link
Contributor

@mn-mikke mn-mikke Apr 11, 2018

Choose a reason for hiding this comment

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

It's already specified in UnaryExpression

@SparkQA
Copy link

SparkQA commented Apr 12, 2018

Test build #89247 has finished for PR 21025 at commit 626e1a8.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 12, 2018

Test build #89251 has finished for PR 21025 at commit d42cdb8.

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

@mgaido91
Copy link
Contributor Author

mgaido91 commented Apr 12, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Apr 12, 2018

Test build #89260 has finished for PR 21025 at commit d42cdb8.

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

@SparkQA
Copy link

SparkQA commented Apr 12, 2018

Test build #89270 has finished for PR 21025 at commit fc51766.

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

@ueshin
Copy link
Member

ueshin commented Apr 13, 2018

LGTM.

gatorsmile
gatorsmile reviewed Apr 13, 2018
View reviewed changes
...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala Outdated
* Returns the minimum value in the array.
*/
@ExpressionDescription(
usage = "_FUNC_(array) - Returns the minimum value in the array.",
Copy link
Member

@gatorsmile gatorsmile Apr 13, 2018

Choose a reason for hiding this comment

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

We will ignore NULL, right? Please document it.

@SparkQA
Copy link

SparkQA commented Apr 13, 2018

Test build #89328 has finished for PR 21025 at commit a7d3a2e.

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

@gatorsmile
Copy link
Member

gatorsmile commented Apr 16, 2018

@mgaido91 Could you address the conflicts? Thanks!

@SparkQA
Copy link

SparkQA commented Apr 16, 2018

Test build #89386 has finished for PR 21025 at commit 1eaaed3.

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

@SparkQA
Copy link

SparkQA commented Apr 16, 2018

Test build #89385 has finished for PR 21025 at commit 638670f.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds the following public classes (experimental):
  • case class ArrayMax(child: Expression) extends UnaryExpression with ImplicitCastInputTypes

@ueshin
Copy link
Member

ueshin commented Apr 17, 2018 •
edited
Loading

The build for the latest commit is 89386 and successfully finished.
Thanks! merging to master.

@asfgit asfgit closed this in 14844a6 Apr 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon left review comments

@gatorsmile gatorsmile gatorsmile left review comments

+1 more reviewer

@mn-mikke mn-mikke mn-mikke left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mgaido91 @SparkQA @gatorsmile @kiszk @ueshin @mn-mikke @HyukjinKwon