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

map SQL dates and times to DateTime #42

Merged
merged 5 commits into from Dec 26, 2022
Merged

map SQL dates and times to DateTime #42

merged 5 commits into from Dec 26, 2022

Conversation

fsw
Copy link
Contributor

@fsw fsw commented Dec 4, 2022

I have tested this just for my simple use case.
@zim32 I can add some more tests for column types in this PR if general approach in this change is OK.
This change is not backward compatible (typedAssoc will return different types)
fixes #12

@zim32
Copy link
Owner

zim32 commented Dec 20, 2022

Hi fsw. ThI think because this library is still not in stable semver we can just bump minor version and write changelog.

I will check your PR in a few days

@fsw
Copy link
Contributor Author

fsw commented Dec 21, 2022

Yes, I guess minor bump would be OK.

I am not sure what to do with sql TIME type, but I guess this one should be passed as String and not converted to DateTime. I will fix this and add some tests.

@zim32
Copy link
Owner

zim32 commented Dec 21, 2022

Yes, TIME should be just String

@fsw
Copy link
Contributor Author

fsw commented Dec 25, 2022

added some changes:

  • map TIME to string
  • map YEAR to int by default (It should fit into integer size)
  • removed test.dart (guess it is unused)
  • refactored tests so socket and TCP connection can be tested using same code base.
  • added test for default types mapping behavior.

Few things I noticed while adding types mapping test:

  • query that returns spatial type is throwing: "FormatException: Unexpected extension byte" I guess this is not yet implemented. I left it commented out and marked with TODO in test code.
  • Types that would not fit into INT (Like BIT, DECIMAL etc) are mapped to String. I guess this is OK if it will be reliable but this creates weird behaviors (like MAX(col) returns int but SUM(col) returns String). Maybe we can consider mapping those types to BigInt?

@zim32
Copy link
Owner

zim32 commented Dec 26, 2022

I think if we convert year to INT now, users will expect that numbers will be converted to numbers or at least to BigInt. So yeah it is logical to convert them to BigInt.

@zim32
Copy link
Owner

zim32 commented Dec 26, 2022

I am merging this PR, but we need to implement BigInt mapping to be consistent.

@zim32 zim32 closed this Dec 26, 2022
@zim32 zim32 reopened this Dec 26, 2022
@zim32 zim32 merged commit 68c3dd2 into zim32:main Dec 26, 2022
@zim32
Copy link
Owner

zim32 commented Dec 26, 2022

You changed something in the tests, so now they are not working.

dart test test/mysql_client.dart

00:00 +0 -1: loading test/mysql_client.dart [E]                                                                                                                           
  Failed to load "test/mysql_client.dart":
  /tmp/dart_test.TYGLEF/test.dart:9:42: Error: Undefined name 'main'.
        internalBootstrapVmTest(() => test.main, sendPort);
                                           ^^^^
00:00 +0 -1: Some tests failed.  

@zim32
Copy link
Owner

zim32 commented Dec 26, 2022

Ah I see. You refactor and move it to separate files for TCP and Unix socket tests. Ok

@fsw
Copy link
Contributor Author

fsw commented Dec 26, 2022

@zim32 yes,

Maybe the file "test/mysql_client.dart" should be moved to "test/lib/mysql_client.dart" or something like this to make it more clear.

And yes the BigInt change can be separate PR.

thanks.

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.

timestamp field mapping
2 participants