-
Notifications
You must be signed in to change notification settings - Fork 109
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
feat: Issue356 db2 test #383
Conversation
9e25713
to
77e69ae
Compare
77e69ae
to
bf767a6
Compare
fixes sum, min, avg, max functions for mysql, ps, db2, and more streamline DB2Client imports
* fix: update spelling * fix:Adding double quote to prevent globbing and word splitting. Adding double quote to prevent globbing and word splitting. * fix:updating comment * fix: Updating inline comments * fix:Spelling * fix:Updating spelling
…Server (#364) * test: get Teradata user name from TERADATA_USER env var * test: add --no-cloud-sql flag to pytest options * test: instantiate CloudSQLResourceManager in a fixture when --no-cloud-sql is not passed * test: optionally get Postgres host from POSTGRES_HOST env var * test: optionally get SQL Server host from SQL_SERVER_HOST env var * test: optionally get SQL server user from SQL_SERVER_USER env var Co-authored-by: A.J. Welch <[email protected]>
* fix: supporting non default schemas for mssql * fix:updated MSSQL client instantiation * fix: typo
* gcs support for validation configs, incl. get and list functionality, and new 'configs' cmd
* adding scaffolding for calc field builder in config manager * exposing cast via calculated fields. Don't know if we necessarily need this just adding for consistency * diff check * config file generating as expected * expanding cli for row level validations * splitting out comparison fields from aggregates * row comparisons operational (sort of) * re-enabling aggregate validations * cohabitation of validation types! * figuring out why unit tests are borked * continuing field split * stash before merge * testing diff * tests passing * removing extra print statements * tests and lint * adding fail tests * first round of requested changes * change requests round two. * refactor CLI and lint * swapping out farm fingerprint for sha256 as default * changes per CR * fixing text result tests * adding docs * hash example * linting * think I found the broken test * fixed tests * setting default for depth length * relaxing system test
* feat: add support for partitioned tables * feat: import schema class * fix: update docs
* adding addons for impala hive hashing functions * fix: import fixed_arity * move logic to ibis_addon * replacing isnull with nvl * adding nvl function * test FillNa * missing import * updating t0 prefix to column names Co-authored-by: Mike Hilton <[email protected]>
0f9716a
to
24d87bb
Compare
/gcbrun |
arg = t.translate(arg) | ||
|
||
#Db2 doesn't allow '*' to be parameterized, probably better way to fix this... | ||
if arg == '*': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is is likely to break anything it touches?
I believe you could pull the column list from arg, but failing that perhaps it makes more sense to raise an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that in other DBMS, you can bind * to a parameter marker, which isn't allowed in aggregation functions in Db2. The ibis code currently doesn't take this into account. This should only run in the case of count(), count() should have only one parameter, so this should have a limited impact. Replacing * with None instead causes the sqlalchemy layer to correctly substitute count(*) explicitly instead of trying to bind * to a parameter which doesn't work in Db2.
Pulling a column list and counting each column separately is not exactly the same as count(*) and would not be an expected outcome.
@ngdav it looks like the integration tests are currently failing due to a stale version of our linting library which has been updated in the main branch. Please merge from develop and commit. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides one doc update - we should add DB2 support to the top level README as well:
https://github.com/GoogleCloudPlatform/professional-services-data-validator/blob/develop/README.md?plain=1#L22
Add DB2 connection to DVT. Connection works