Skip to content
This repository was archived by the owner on May 17, 2024. It is now read-only.

Duckdb driver for Issue #176 #276

Merged
merged 1 commit into from
Nov 15, 2022
Merged

Duckdb driver for Issue #176 #276

merged 1 commit into from
Nov 15, 2022

Conversation

jardayn
Copy link
Contributor

@jardayn jardayn commented Nov 4, 2022

Copy link
Contributor

@erezsh erezsh left a comment

Choose a reason for hiding this comment

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

Good start!

@erezsh
Copy link
Contributor

erezsh commented Nov 4, 2022

Read the docstring, you need to implement rounds: https://github.com/datafold/data-diff/blob/master/data_diff/databases/database_types.py#L213

It may pass your local tests, but if you test it vs Snowflake it will fail otherwise.

@erezsh
Copy link
Contributor

erezsh commented Nov 10, 2022

You're changing a lot of lines you're not supposed to touch, mostly with whitespace - https://github.com/datafold/data-diff/pull/276/files

And I don't think test_duckdb is actually testing anything that isn't already covered, is it?

@jardayn
Copy link
Contributor Author

jardayn commented Nov 10, 2022

Ran black -l 120. Removed test_duckdb.py

@jardayn jardayn marked this pull request as ready for review November 10, 2022 19:55
Copy link
Contributor

@erezsh erezsh left a comment

Choose a reason for hiding this comment

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

Also, for some reason looks like test_duckdb is still in the PR?

@jardayn jardayn requested a review from erezsh November 10, 2022 20:16
@erezsh
Copy link
Contributor

erezsh commented Nov 10, 2022

@jardayn I tried to run this locally and got this error:

duckdb.ConversionException: Conversion Error: Could not convert string '0x90762715a289763' to INT64

Test: test_types_duckdb_bigint_duckdb_bigint_50
duckdb.version = '0.5.2-dev1579'

@jardayn
Copy link
Contributor Author

jardayn commented Nov 10, 2022

Waiting on DuckDB 0.6.0. that'll bring HEX to int conversions.

Copy link
Contributor

@erezsh erezsh left a comment

Choose a reason for hiding this comment

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

A few comments, but overall looks good!

And please also revert the changes waiting_for_stack_up.sh

Added MD5 to Int for DDB

Upped the amount of queries for the test

Normalization

Dialect + Match Path

No more match path

Now DDB is part of the tests

Trying to get tests to work

More debugging of tests

More types

Tests work!

cleanup

fixed conflict

Added Boolean tests to DDB

Added Boolean tests to DDB

black -l 120 formatting

Removed test_duckdb

Reverting formatting

Review comments

Review comments

0.6.0. DDB!

TSTZ tests for DDB fixed

Tests for DDB work

changed how test_cli gets the current TZ

Review comments

reverting changes to test_cli
Copy link
Contributor

@erezsh erezsh left a comment

Choose a reason for hiding this comment

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

Looks good!

@erezsh erezsh merged commit e558259 into datafold:master Nov 15, 2022
@erezsh
Copy link
Contributor

erezsh commented Nov 15, 2022

Thank you for contributing! 🥳

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants