Skip to content

[CALCITE-6483] Enable LEN and LENGTH in the correct function libraries#3865

Merged
rubenada merged 1 commit intoapache:mainfrom
Bit-Quill:calcite-6483
Jul 21, 2024
Merged

[CALCITE-6483] Enable LEN and LENGTH in the correct function libraries#3865
rubenada merged 1 commit intoapache:mainfrom
Bit-Quill:calcite-6483

Conversation

@normanj-bitquill
Copy link
Contributor

Enabled the LEN function for:

  • Amazon Redshift
  • Snowflake
  • Spark

Enabled the LENGTH function for:

  • BigQuery
  • Hive
  • PostgreSQL
  • Amazon Redshift
  • Snowflake
  • Spark

@caicancai
Copy link
Member

Should we add tests to SqlOperatorTest?

@rubenada
Copy link
Contributor

Should we add tests to SqlOperatorTest?

@caicancai I think there is no need to touch SqlOperatorTest. The way that class works, the already existing tests testLenFunc and testLengthFunc will automatically consider the libraries in the @LibraryOperator annotation of LEN and LENGTH (including the newly added ones). So new checks are added transparently for the new libraries.

@rubenada
Copy link
Contributor

@normanj-bitquill overall LGTM. I think that current patch is wider than the original intent, so perhaps we should adjust the Jira title (and commit message) to something broader, e.g. "Add LEN and LENGTH functions to missing libraries", wdyt?

@caicancai
Copy link
Member

@normanj-bitquill overall LGTM. I think that current patch is wider than the original intent, so perhaps we should adjust the Jira title (and commit message) to something broader, e.g. "Add LEN and LENGTH functions to missing libraries", wdyt?

@rubenada Thank you for your reply. I am worried that in some negative tests, the results of each dialect may be different. Do you mind if I test it on some databases first?

Copy link
Member

@caicancai caicancai left a comment

Choose a reason for hiding this comment

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

LGTM, I test negative test

Enabled the LEN function for:
* Amazon Redshift
* Snowflake
* Spark

Enabled the LENGTH function for:
* BigQuery
* Hive
* PostgreSQL
* Amazon Redshift
* Snowflake
* Spark
@normanj-bitquill normanj-bitquill changed the title [CALCITE-6483] Add length function to PostgreSQL function library [CALCITE-6483] Enable LEN and LENGTH in the correct function libraries Jul 19, 2024
@normanj-bitquill
Copy link
Contributor Author

@rubenada I have updated the ticket title and commit message. Let me know if it still doesn't match the change.

@sonarqubecloud
Copy link

Copy link
Contributor

@rubenada rubenada left a comment

Choose a reason for hiding this comment

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

LGTM

@rubenada rubenada added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 20, 2024
@rubenada
Copy link
Contributor

Thanks @normanj-bitquill , LGTM. I'll merge in the next 24-48h if no further comment appears.

@rubenada rubenada merged commit ed1f54f into apache:main Jul 21, 2024
@normanj-bitquill normanj-bitquill deleted the calcite-6483 branch August 27, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants