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: implements ARRAY_JOIN as requested in (#5028) #5474

Merged
merged 2 commits into from
Jun 12, 2020

Conversation

hpgrahsl
Copy link
Contributor

Description

This PR implements the UDF / scalar function ARRAY_JOIN as requested in (#5028). It creates a flat string representation of all the elements contained in the given array.

Testing done

There are unit tests with several applications of the UDF to verify that it follows the expected behaviour as discussed in the linked issue.

@hpgrahsl hpgrahsl requested review from JimGalasyn and a team as code owners May 24, 2020 12:34
@ghost
Copy link

ghost commented May 24, 2020

@confluentinc It looks like @hpgrahsl just signed our Contributor License Agreement. 👍

Always at your service,

clabot

Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty good! Always happy to see you contributing @hpgrahsl 😂

It is missing what we call "QueryTranslationTests", you can see an example of one in substring.json. After you write this file (you can actually move the more complex tests in ArrayJoinTest.java to this file) you will need to generate the plans that we use to make sure things stay backwards compatible across versions. To do that, simply un-ignore and run PlannedTestGeneratorTest.

@agavra agavra requested a review from a team May 26, 2020 16:23
@hpgrahsl
Copy link
Contributor Author

It is missing what we call "QueryTranslationTests", you can see an example of one in substring.json

I have to look into this and might have further questions most likely about this. I wrote the tests in relation to other, existing array function tests and haven't seen anything like this.

Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

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

LGTM, with a couple of suggestions!

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.

@hpgrahsl thank you for the excellent contribution!

hpgrahsl added a commit to hpgrahsl/ksql that referenced this pull request Jun 12, 2020
as requested by reviewers:

- removed BigInteger from primitive types
- added Long and BigDecimal to supported primitive types
- dropped support for complex nested types
- switched to hamcrest matchers in unit tests
- added QueryTranslationTest for via PlannedTestGeneratorTest
- adapted docs accordingly
as requested by reviewers:

- removed BigInteger from primitive types
- added Long and BigDecimal to supported primitive types
- dropped support for complex nested types
- switched to hamcrest matchers in unit tests
- added QueryTranslationTest for via PlannedTestGeneratorTest
- adapted docs accordingly
@hpgrahsl
Copy link
Contributor Author

as requested by reviewers:

  • removed BigInteger from primitive types
  • added Long and BigDecimal to supported primitive types
  • dropped support for complex nested types
  • switched to hamcrest matchers in unit tests
  • added QueryTranslationTest for via PlannedTestGeneratorTest
  • adapted docs accordingly

@hpgrahsl hpgrahsl requested review from agavra and removed request for a team June 12, 2020 13:07
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for the contribution @hpgrahsl

@agavra agavra merged commit 06e1df6 into confluentinc:master Jun 12, 2020
stevenpyzhang added a commit that referenced this pull request Jun 17, 2020
Co-authored-by: Hans-Peter Grahsl <hpgrahsl@users.noreply.github.com>
JimGalasyn added a commit that referenced this pull request Jun 25, 2020
* feat: implements ARRAY_JOIN as requested in (#5028) (#5474) (#5638)

Co-authored-by: Hans-Peter Grahsl <hpgrahsl@users.noreply.github.com>

* feat: new split_to_map udf (#5563)

New UDF split_to_map(input, entryDelimiter, kvDelimiter) to build a map from a string.

Useful for taking messages from upstream systems and converting them into a more structured and usable format.

* feat: add CHR UDF (#5559)

A new UDF, CHR, to turn a number representing a unicode codepoint into a single-character string. Very useful for dealing with non-printable characters (tab, CR, LF, ...) in strings or those characters not easily represented in your local codepage.

Co-authored-by: Steven Zhang <35498506+stevenpyzhang@users.noreply.github.com>
Co-authored-by: Hans-Peter Grahsl <hpgrahsl@users.noreply.github.com>
Co-authored-by: Nick Dearden <blueedgenick@users.noreply.github.com>
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.

None yet

4 participants