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.

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

[CALCITE-6315] Support PostgreSQL TO_CHAR, TO_DATE, TO_TIMESTAMP #3753

Merged
merged 1 commit into from Apr 9, 2024
Merged

[CALCITE-6315] Support PostgreSQL TO_CHAR, TO_DATE, TO_TIMESTAMP #3753

merged 1 commit into from Apr 9, 2024

Conversation

normanj-bitquill
Copy link
Contributor

@normanj-bitquill normanj-bitquill commented Apr 5, 2024 •
edited

  • Both functions use PostgreSQL format patterns
  • Added tests for format patterns supported by PostgreSQL but missing from Calcite
  • If the data or timestamp cannot be parsed using format string, then an exception is thrown.

@mihaibudiu
Copy link
Contributor

The style checker has some complaints about your code. You can see them in the failed CI run.

@normanj-bitquill
Copy link
Contributor Author

@mihaibudiu Thank you, I have addressed the style checker error. The latest CI run failed when trying to pull the redis docker image.

mihaibudiu
mihaibudiu reviewed Apr 8, 2024
View reviewed changes
Copy link
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

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

I don't understand what happens to TO_CHAR in this PR.

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java Outdated Show resolved Hide resolved
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java Show resolved Hide resolved
@normanj-bitquill normanj-bitquill changed the title [CALCITE-6315] Add TO_CHAR, TO_DATE, TO_TIMESTAMP functions [CALCITE-6315] Add TO_DATE, TO_TIMESTAMP functions Apr 9, 2024
@normanj-bitquill
Copy link
Contributor Author

I don't understand what happens to TO_CHAR in this PR.

@mihaibudiu Good point. I have updated the title of this PR and the commit message to say that I only added support for TO_DATE and TO_TIMESTAMP.

@mihaibudiu
Copy link
Contributor

In order to keep track of JIRA/github, we require the JIRA title to be the same as the PR title, and the commit message.
Can you please make sure they match?
The JIRA title is good if you remove "TO_CHAR" from it.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Apr 9, 2024

Quality Gate Passed Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@normanj-bitquill
Copy link
Contributor Author

In order to keep track of JIRA/github, we require the JIRA title to be the same as the PR title, and the commit message. Can you please make sure they match? The JIRA title is good if you remove "TO_CHAR" from it.

@mihaibudiu I don't have permission to change the title in Jira or to assign the ticket to myself. Can I get the necessary permissions?

@mihaibudiu
Copy link
Contributor

I edited the JIRA title. You shoud change the commit and PR titles.
But if you have a JIRA account and you filed the issue I would expect you to be able to edit it... I don't know much about JIRA, though. Don't worry about assigning it.

@normanj-bitquill
[CALCITE-6315] Support PostgreSQL TO_CHAR, TO_DATE, TO_TIMESTAMP
12a505e 
* Both functions use PostgreSQL format patterns
* Added tests for format patterns supported by PostgreSQL but missing from Calcite
* If the data or timestamp cannot be parsed using format string, then an exception
  is thrown.
@normanj-bitquill normanj-bitquill changed the title [CALCITE-6315] Add TO_DATE, TO_TIMESTAMP functions [CALCITE-6315] Support PostgreSQL TO_CHAR, TO_DATE, TO_TIMESTAMP Apr 9, 2024
@normanj-bitquill
Copy link
Contributor Author

@mihaibudiu Thanks for changing the title in Jira. I have updated the commit message and title of this PR. I'll sort out my Jira permissions.

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Apr 9, 2024
@mihaibudiu mihaibudiu merged commit 889f416 into apache:main Apr 9, 2024
16 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihaibudiu mihaibudiu mihaibudiu approved these changes

Assignees
No one assigned
Labels
LGTM-will-merge-soon Overall PR looks OK. Only minor things left.
Projects
None yet
Milestone
No milestone
2 participants
@normanj-bitquill @mihaibudiu