Skip to content
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.

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

feat: new UDFs for array max/min/sort #5505

Merged
merged 4 commits into from
May 29, 2020

Conversation

blueedgenick
Copy link
Contributor

Description

Three new UDFs for array manipulation:

  • array_min
  • array_max
  • array_sort

Docs updated.

Testing done

Accompanying unit and QTT tests added.

@blueedgenick blueedgenick requested review from JimGalasyn and a team as code owners May 28, 2020 21:32
Copy link
Contributor

@derekjn derekjn left a comment

Choose a reason for hiding this comment

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

Awesome work, thank you @blueedgenick!


@Udf
public <T extends Comparable<? super T>> T arrayMax(@UdfParameter(
description = "The array to sort") final List<T> input) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this isn't the intended description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, thx!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed


@Udf
public <T extends Comparable<? super T>> T arrayMin(@UdfParameter(
description = "The array to sort") final List<T> input) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this isn't the intended description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

### ``ARRAY_SORT``

```sql
ARRAY_SORT(["foo", "bar", "baz"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this array literal isn't intended to be a valid argument, but it may be worth using single quotes here since ARRAY["foo", "bar"] wouldn't parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed all the occurrences i could find

ARRAY_SORT(["foo", "bar", "baz"])
```

Given an array of primitive elements (not arrays of other arrays, or maps, or structs, or combinations thereof), returns an array of the same elements sorted according to their natural sort order. Any NULLs contained in the array will always be moved to the end.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be worth adding a note and example or two about the optional ASC or DESC order specifier since that's pretty useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, oversight on my part. adding now

@blueedgenick
Copy link
Contributor Author

should have added the linked issues:
addresses #5488 for array-min/max
addresses #5490 for array_sort

Copy link
Member

@vpapavas vpapavas left a comment

Choose a reason for hiding this comment

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

Thank you @blueedgenick! Awesome job!
I forgot to say, before you merge, you need to add the subfoler created in ksql/ksqldb-functional-tests/src/test/resources/historical_plans for your QTT test. There should be three files, the spec.json, the plan.json and the topology. If you need to create them manually, you can run PlannedTestGeneratorTest.manuallyGeneratePlans which will generate plans for all QTT tests .

@vpapavas vpapavas self-requested a review May 29, 2020 19:22
Copy link
Member

@vpapavas vpapavas left a comment

Choose a reason for hiding this comment

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

LGTM!

@blueedgenick blueedgenick merged commit 415d930 into confluentinc:master May 29, 2020
@blueedgenick blueedgenick deleted the new_array_udfs branch June 1, 2020 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants