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

[Feature][Zeta] add from_unixtime function #5462

Merged
merged 11 commits into from
Nov 20, 2023

Conversation

chovy-3012
Copy link
Contributor

Purpose of this pull request

Check list

add from_unixtime function.
Convert the number of seconds from the UNIX epoch (1970-01-01 00:00:00 UTC) to a string representing the timestamp of that moment.

@chovy-3012 chovy-3012 changed the title [feature] add from_unixtime function [Feature][Zeta] add from_unixtime function Sep 11, 2023
@Hisoka-X
Copy link
Member

cc @rewerma

Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

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

How do you test it? Could you add some test case?

Copy link
Contributor Author

@chovy-3012 chovy-3012 Sep 14, 2023

Choose a reason for hiding this comment

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

yes , I've already added unit test for from_unixtime function

Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

Please add doc about this feature. https://seatunnel.apache.org/docs/2.3.3/transform-v2/sql-functions

After this PR is merged, we may need to consider adding benchmark test for the function.

ruanwenjun
ruanwenjun previously approved these changes Sep 14, 2023
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

LGTM

@Hisoka-X
Copy link
Member

Seem like the code style should be fixed. Please run mvn spotless:apply to fix the code style.

@chovy-3012
Copy link
Contributor Author

chovy-3012 commented Sep 14, 2023

Seem like the code style should be fixed. Please run mvn spotless:apply to fix the code style.

missed code formatting on sql-functions.md

  • spotless:apply on doc

@Hisoka-X
Copy link
Member

The UT not passed. @chovy-3012 Could you check it?

[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    DateTimeFunctionTest.testFromUnixtimeFunction:53 expected: <2023-01-01 00:00:00> but was: <2022-12-31 16:00:00>

@chovy-3012
Copy link
Contributor Author

The UT not passed. @chovy-3012 Could you check it?

[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    DateTimeFunctionTest.testFromUnixtimeFunction:53 expected: <2023-01-01 00:00:00> but was: <2022-12-31 16:00:00>

I made a mistake on the results of this unit test on host in different time zones.
Your host should be in the UTC+0 time zone.
I have modified the unit test.

@ruanwenjun
Copy link
Member

Please resolve the conflict.

@@ -76,6 +76,7 @@
- [Zeta] Fix cpu load problem (#4828)
- [zeta] Fix the deadlock issue with JDBC driver loading (#4878)
- [zeta] dynamically replace the value of the variable at runtime (#4950)
- [Zeta] Add from_unixtime function (#5462)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

conflict resolved

@rewerma
Copy link
Contributor

rewerma commented Sep 18, 2023

Please add e2e test case.

@chovy-3012
Copy link
Contributor Author

add e2e test case

@Hisoka-X
Copy link
Member

The CI failed. Could you investigate it?

@hailin0 hailin0 closed this Oct 25, 2023
@hailin0 hailin0 reopened this Oct 25, 2023
@chovy-3012
Copy link
Contributor Author

Retried more than 10 times , still failed.
Error messages show no space left on device , any suggestions?

error message as below:

[INFO] Running org.apache.seatunnel.connectors.seatunnel.jdbc.JdbcAutoGenerateSQLIT
Error:  Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 8.34 s <<< FAILURE! - in org.apache.seatunnel.connectors.seatunnel.jdbc.JdbcAutoGenerateSQLIT
Error:  org.apache.seatunnel.connectors.seatunnel.jdbc.JdbcAutoGenerateSQLIT  Time elapsed: 8.34 s  <<< ERROR!
java.util.concurrent.CompletionException: org.testcontainers.containers.ContainerLaunchException: Container startup failed
Caused by: org.testcontainers.containers.ContainerLaunchException: Container startup failed
Caused by: org.testcontainers.containers.ContainerFetchException: Can't get Docker image: RemoteDockerImage(imageName=postgres:14-alpine, imagePullPolicy=DefaultPullPolicy(), imageNameSubstitutor=org.testcontainers.utility.ImageNameSubstitutor$LogWrappedImageNameSubstitutor@489496a9)
Caused by: org.testcontainers.containers.ContainerFetchException: Failed to get Docker client for postgres:14-alpine
Caused by: com.github.dockerjava.api.exception.DockerClientException: Could not pull image: failed to register layer: write /usr/lib/libLLVM-15.so: no space left on device

Copy link
Member

@Carl-Zhou-CN Carl-Zhou-CN left a comment

Choose a reason for hiding this comment

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

+1

@Hisoka-X
Copy link
Member

Hisoka-X commented Nov 2, 2023

Hi @chovy-3012 , could you try merge from dev to retrigger CI? Thanks.

@chovy-3012
Copy link
Contributor Author

chovy-3012 commented Nov 4, 2023

Hi @chovy-3012 , could you try merge from dev to retrigger CI? Thanks.

@Hisoka-X
Merge to my this feature branch from branch apache dev
Wouldn't this result in the feature branch having many commits that have already been merged into the dev branch?

@Carl-Zhou-CN
Copy link
Member

@chovy-3012
image
You can start with the branch chovy-3012:feature/from_unixtime_function

@Carl-Zhou-CN
Copy link
Member

Carl-Zhou-CN commented Nov 14, 2023

image
@chovy-3012 Sorry, that's how it's supposed to work. I made a mistake

@chovy-3012
Copy link
Contributor Author

Merged branch apache:dev into my:feature/from_unixtime_function , but CI still has one error after multiple retries
@Carl-Zhou-CN

Copy link
Member

@Carl-Zhou-CN Carl-Zhou-CN left a comment

Choose a reason for hiding this comment

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

LGTM

@EricJoy2048 EricJoy2048 merged commit fb24f8e into apache:dev Nov 20, 2023
6 of 7 checks passed
alextinng pushed a commit to alextinng/seatunnel that referenced this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants