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

Add support for more value types in TrinoBatchInsert #24

Merged
merged 7 commits into from
Mar 30, 2022

Conversation

MichaelTiemannOSC
Copy link
Contributor

Seems needed by Trino:

Works:
('Exelon Corp.', 'Commonwealth Edison Co.', 32, TIMESTAMP '2015-01-01 00:00:00', 'nuclear', 'non_fuel_operation_expenses', 'electric expenses', 0.0, 0.0)

Fails:
('Exelon Corp.', 'Commonwealth Edison Co.', 32, 2015-01-01 00:00:00, 'nuclear', 'non_fuel_operation_expenses', 'electric expenses', 0.0, 0.0)

Seems needed by Trino:

Works:
  ('Exelon Corp.', 'Commonwealth Edison Co.', 32, TIMESTAMP '2015-01-01 00:00:00', 'nuclear', 'non_fuel_operation_expenses', 'electric expenses', 0.0, 0.0)

Fails:
  ('Exelon Corp.', 'Commonwealth Edison Co.', 32, 2015-01-01 00:00:00, 'nuclear', 'non_fuel_operation_expenses', 'electric expenses', 0.0, 0.0)
Signed-off-by: MichaelTiemannOSC <mtiemann@os-climate.org>
@MichaelTiemannOSC MichaelTiemannOSC removed the request for review from erikerlandson March 28, 2022 22:07
@MichaelTiemannOSC MichaelTiemannOSC added the bug Something isn't working label Mar 28, 2022
@MichaelTiemannOSC
Copy link
Contributor Author

I reviewed the log but don't understand what is causing the failure. The test code I see looks innocuous.

@erikerlandson
Copy link
Contributor

ERROR: /home/runner/work/osc-ingest-tools/osc-ingest-tools/osc_ingest_trino/trino_utils.py Imports are incorrectly sorted and/or formatted.

You have to pay the formatter troll a coin. Use isort ., see here for more details:
https://github.com/os-climate/osc-ingest-tools#development

Use isort to put dependencies in proper order.

Signed-off-by: MichaelTiemannOSC <mtiemann@os-climate.org>
Trino prefers to see NaN as nan()

Signed-off-by: MichaelTiemannOSC <mtiemann@os-climate.org>
And also handle things that are math.isnan correctly.

Signed-off-by: MichaelTiemannOSC <mtiemann@os-climate.org>
Corrected.

Signed-off-by: MichaelTiemannOSC <mtiemann@os-climate.org>
@erikerlandson
Copy link
Contributor

We need to add timestamp and NaN values to unit tests, to verify they are being output in the way you expect
https://github.com/os-climate/osc-ingest-tools/blob/main/tests/test_trino_utils.py#L34

Support non-finite floats, in case needed.  The SQL spelling of nan, inf, and -inf is different.

Signed-off-by: MichaelTiemannOSC <mtiemann@os-climate.org>
It's "boolean" in SQL-land.

Signed-off-by: MichaelTiemannOSC <mtiemann@os-climate.org>
@erikerlandson erikerlandson changed the title Added TIMESTAMP prefix when value is a datatime Add support for more value types in TrinoBatchInsert Mar 30, 2022
@erikerlandson erikerlandson merged commit 0dd10ee into main Mar 30, 2022
erikerlandson added a commit that referenced this pull request Mar 30, 2022
Signed-off-by: Erik Erlandson <eerlands@redhat.com>
erikerlandson added a commit that referenced this pull request Mar 31, 2022
@erikerlandson erikerlandson deleted the fix-timestamp branch March 31, 2022 22:59
erikerlandson added a commit that referenced this pull request Apr 20, 2022
Signed-off-by: Erik Erlandson <eerlands@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants