fix issue-3865 mapping DATETIME of MySQL to corresponding type to Pos…

[Morpheus1999]

…tgres

What

Describe what the change is solving
It helps to add screenshots if it affects the frontend.
Adding type of Timestamp support in MySQL source connector, which help to map corresponding type in Postgres destination side.

How

Describe the solution
Adding new case branch in getJsonType method
Adding new case branch in JsonSchemaPrimitive.java for generating correct catalog file

Recommended reading order

1. MySqlSourceOperations.java
2. JsonSchemaPrimitive.java

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

• Community member? Grant edit access to maintainers (instructions)
• Secrets in the connector's spec are annotated with airbyte_secret
• Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
• Code reviews completed
• Documentation updated
  • Connector's README.md
  • Connector's bootstrap.md. See description and examples
  • docs/SUMMARY.md
  • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • docs/integrations/README.md
  • airbyte-integrations/builds.md
• PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

• Create a non-forked branch based on this PR and test the below items on it
• Build is successful
• Credentials added to Github CI. Instructions.
• /test connector=connectors/<name> command is passing.
• New Connector version released on Dockerhub by running the /publish command described here
• After the connector is published, connector added to connector index as described here
• Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

• Grant edit access to maintainers (instructions)
• Secrets in the connector's spec are annotated with airbyte_secret
• Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
• Code reviews completed
• Documentation updated
  • Connector's README.md
  • Connector's bootstrap.md. See description and examples
  • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
• PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

• Create a non-forked branch based on this PR and test the below items on it
• Build is successful
• Credentials added to Github CI. Instructions.
• /test connector=connectors/<name> command is passing.
• New Connector version released on Dockerhub by running the /publish command described here
• After the new connector version is published, connector version bumped in the seed directory as described here
• Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Connector Generator

• Issue acceptance criteria met
• PR name follows PR naming conventions
• If adding a new generator, add it to the list of scaffold modules being tested
• The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
• Documentation which references the generator is updated as needed.

[CLAassistant]

All committers have signed the CLA.

[harshithmullapudi]

Hey @Morpheus1999 thanks for the contribution. Can you help with screenshots of sourceAcceptanceTests also you check this https://docs.airbyte.com/connector-development#publishing-a-connector regarding bumping the version

[Morpheus1999]

Dear @harshithmullapudi
Thank you for your reply. Please kindly find sourceAcceptanceTest result below. I follow the instruction of Mysql Source connecter page and ran integreationTest command.

For dataTypeTest which same as this change expected.

For last two fails, I saw the reason is one file cannot be found, which I cannot figuring out the root reason, since I did not change other things except for data types. I also run integrationTest in origin Master which got same error. See following screenshot.

I also change version to 0.5.2 in Dockerfile.

Please kindly help to review it. If anything need to be performed by me, please inform me, thanks. Since this is my first time to submit PR, it may need more guidance , thank you for your patience.

[harshithmullapudi]

Hey, will look into this today. Also, can you ask root to sign the CLA.

[Morpheus1999]

Dear @harshithmullapudi

I am afraid I cannot use root to sign the CLA, it may casued by I use wrong Linux user to commit the code. It would be better to rollback the latest commit and recommit it use my own account on linux.

[Morpheus1999]

Dear @harshithmullapudi

I have recommit it and fix CLA issue. Please kindly review it.

Thanks

[harshithmullapudi]

Sure will take a look at it EOD

[Morpheus1999]

Dear @harshithmullapudi

I am glad to see this pull request was approved, but I saw this pull request was blocked by some checks. Should I do something for this or just waiting for other people who have authentication to merge this request.
Will I become a contributor, after this pull request completed?

[harshithmullapudi]

Yeah I will run the tests and check the acceptance tests today.

[Morpheus1999]

Dear @harshithmullapudi
Thank you so much.

[Morpheus1999]

Dear @harshithmullapudi

I just resolve one conflict which caused by label version in Dockerfile. I think new version docker file had been pushed to master by other contributors during pull request period. Please kindly help to resolve sonar scan failed check.

Thank you so much.

[harshithmullapudi]

@tuliren can you give it a quick review here (since you have more idea on the types and you have worked on it before). Just needed a small +1 from you

Just being more curious when you handled all types why didn't we handle this also?

[tuliren]

We are already working on a more generic solution for the date and time types. The most recent PR is #9913. Sorry that I'd prefer to not merge this PR, as it will be superseded by our internal solution.

[Morpheus1999]

@tuliren @harshithmullapudi
I am glad to see there will be a solution of this issue. Does it mean this pr and its effort become nothing, just because there will be another pr in the future？

[Morpheus1999]

Dear @tuliren @harshithmullapudi

I just quickly view PR which mentioned by @tuliren #9913 . I cannot find type of timestamp or datetime were handled in following screenshot.

It would be better to merge this PR #9891 or #10305 and perform other reconstruction in future PR. Since #9913 was behind #9891 .
Thanks

[tuliren]

Right, #9913 is the first step for this big type change. It does not add date and time supports yet.

We need to evaluate the impact of this change to existing users before merging this. I will look more into this tomorrow.

Don't worry, this PR will be merged. If there is any merge conflict, I can fix that.

[sherifnada]

@Morpheus1999 thanks for making the change! this change is a bit more involved for the reasons outlined here #8280, so before the mysql source can start declaring format: date-time we first need to make a distinction around timezone awareness in normalization, as today it doesn't handle that (second step in the checklist in the linked ticket).

can we put this PR on hold until the list of blocking issues are addressed?

[Morpheus1999]

@sherifnada I have carefully read comments in #8280 . Currently, the normalization part just convert data, which has format "date-time", to timestamp with timezone regardless of whether the source columns have timezone or not. Does this PR have conflict with solution of issue #8280 mentioned by you？

[sherifnada]

@Morpheus1999 you'll notice no database source currently declares columns with format: date-time due to the timezone awareness issue described. Looking at the MySQL docs, I actually am not 100% sure whether TIMESTAMP should be interpreted as -with-timestamp or -without. I think it should be with, but we need to make sure connection timezone is appropriately set before declaring columns as being timezone-with-timestamp. Is this piece clear to you?

[Morpheus1999]

@sherifnada I understand your point. I Just want to figure out what should I do for helping this issue?

[harshithmullapudi]

@Morpheus1999 Here is the update on this

As per the reasons described here this change alone is insufficient #8280 you can take a look at it and feel free to continue the contribution.

[harshithmullapudi]

I will be closing this PR. Feel free to reach out to me if you need any help. Thanks a lot.

[octavia-squidington-iii]

Marcos Marx commented: Dear @harshithmullapudi

I just resolve one conflict which caused by label version in Dockerfile. I think new version docker file had been pushed to master by other contributors during pull request period. Please kindly help to resolve sonar scan failed check.

Thank you so much.

[octavia-squidington-iii]

Marcos Marx commented: @Morpheus1999 thanks for making the change! this change is a bit more involved for the reasons outlined here #8280, so before the mysql source can start declaring format: date-time we first need to make a distinction around timezone awareness in normalization, as today it doesn't handle that (second step in the checklist in the linked ticket).

can we put this PR on hold until the list of blocking issues are addressed?

[octavia-squidington-iii]

Marcos Marx commented: @sherifnada I understand your point. I Just want to figure out what should I do for helping this issue?