-
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
generate-table-partitions
generates invalid SQL in filter clause when a timestamp field is part of the primary-key
#923
Comments
Hi @florisvink, thanks for reporting this! Could you please help us understand how much this is affecting you at the moment? We did a triage for this problem and we will tackle it as soon as we can, since we just merged some updates for this feature on #922. |
The problem is in two static methods
This works correctly if the type is either string or numeric (real, integer etc). It does not work when the value is of type Boolean, datetime or interval(?). The fix is to change this to cover those use cases. While the code change is straightforward, we also need to come up with a test case(s) with primary keys with those column types and test across all the different databases. Sundar Mudupalli |
Hi @helensilva14, thanks for your reply. We have a workaround for the issue on short term (a regex string-replace on all partitioned config files fixing the quotes for datetime values). I'll keep an eye on updates in the |
I also found out that this quoting problem appears when a STRING-field is part of the primary key and it contains a quote itself: See
|
Hi @florisvink @helensilva14 |
That's very helpful @piyushsarraf . I was fixing those issues now by hand as well by adding an explicit type-cast on date-time columns. |
Many thanks for working on this! Example:
(see |
Hi @florisvink! This new PR #962 is the most up-to-date one to fix the filter issue (#950) and our testings are almost finished. But it is addressing this TIMESTAMP issue, partially because of a dependency version we currently use which is SQLAlchemy==1.4.49 (current version)
SQLAlchemy==2.0.20 (recommended and tested version that works!)
Steps that will be needed after #962 is merged:
Let us know if you need direct assistance from us for any of this! |
* fix: Change in filter tag creation * fix: Change in test files to integrate double quotes in string * fix: Minor Fix * fix: Fixing lint error * fix: Fixing linting issue * fix: typo * Fix: fixing date issue and removing string logic and its tests * fix: suggested changes * feat: Adding integration tests for BQ, Hive, Teradata * Update partition_table_prd.md potential way to address #923 and #950 * Fixed bug with specifying a config file in GCS - this did not work earlier Added functionality to support Kubernetes Indexed jobs - which when provided with a directory will only run the job corresponding to the index. Tested in a non Kubernetes setup * Updated function to create the filter logic by using the feature of ibis to turn table expressions into SQL statements. This addresses bugs #945 and #950. Unfortunately, we depend on the version of sqlalchemy being 2.0 or later which has fixed a problem with datetime being rendered by compile - see https://docs.sqlalchemy.org/en/20/changelog/changelog_20.html#change-206ec1f2af3a0c93785758c723ba356f * Changes to fix issues 945 and 950, updated documentation, internal PRD and test cases. Need to check that everything works. * Back out changes made for Kubernetes and by earlier attempts to fix #945 and #950 * Backout some changes that were made in other branches * Modified tests since the where clause is now syntactically slightly different, though semantically the same Cleaned up tests and partition_builder to pass flake8 and black * Fixed issue with SQL Server test * Fix string comparison issues * Fixed flake8 issues * Update Snowflake partition filter and confirmed that it works in local test --------- Co-authored-by: Piyush Sarraf <[email protected]>
After pulling latest Command:
Result:
|
@florisvink The 'standard_conforming_strings' error is because the redshift sqlalchemy connector doesn't support sqlalchemy versions 2.0: sqlalchemy-redshift/sqlalchemy-redshift#264 Due to this package dependency conflict, there isn't a way forward to support Redshift using the to_sql() method. This would probably require reverting the change and adding a condition to format timestamps. |
When I used the latest filters:
- source: orderdatetime > '2023-02-15'
target: orderdatetime > '2023-02-15'
type: custom
- source: ' orderdatetime > ''2023-02-15'' AND orderid >= ''6dff79b1c7ffe28f29a44f9476a26b0e53edffa76d1fe06a8f7ce6252a7338df''
AND orderid < ''6e288c21a16d73502110194c2fad6010868a1d58caf3c0e382b4b0a7262bef56'''
target: ' ( orderdatetime > ''2023-02-15'' ) AND ( `orderid` >= ''6dff79b1c7ffe28f29a44f9476a26b0e53edffa76d1fe06a8f7ce6252a7338df''
) AND ( `orderid` < ''6e288c21a16d73502110194c2fad6010868a1d58caf3c0e382b4b0a7262bef56''
)'
I guess I could try to string-replace the |
Sorry, ignore comment above. Although it looks wrong it does actually works with double single quotes. |
As Neha mentioned, we are not able to support timestamp in a composite primary key because RedShift does not work with SQLAlchemy 2.0. I am not sure you need to specify timestamp as a part of the composite primary key. In a traditional transactional databases a primary key (or composite primary key) is used to uniquely identify a row. This is how we use the term Primary Keys in the data validator. Our documentation says the keys uniquely identify the row. Generally using a timestamp in composite primary key in a transactional database is not a good idea as discussed here In AWS Redshift, the primary keys are informational only, they are used for query optimization as mentioned in the documentation Therefore, the only columns you need to mention in the primary key for data validator are the keys that uniquely identify the row. Hopefully you don't need to specify timestamp and therefore this is not an issue for you. At least that is my hope. Sundar Mudupalli |
Hi everyone! Updating this issue to reinforce that we encountered this problem above, but since @florisvink is not blocked anymore, I changed it to a feature request for us to get back here to test the feature when the sqlalchemy-redshift/sqlalchemy-redshift#264 gets solved and Redshift starts to support SQLAlchemy 2.0+. Thanks a lot everyone for all the effort! |
Being able to cast values while generating partitions would also solve a lot of problems while generating partitions. For instance we would have be able to cast a DATE to a string which when sorted would have the same behaviour |
* Update partition_table_prd.md potential way to address #923 and #950 * Fixed bug with specifying a config file in GCS - this did not work earlier Added functionality to support Kubernetes Indexed jobs - which when provided with a directory will only run the job corresponding to the index. Tested in a non Kubernetes setup * Added a Implementation Note on how to scale DVT with Kubernetes Shortened the option to 2 character code -kc * Linted files and added unit tests for config_runner changes. * Updated README on how to run multiple instances concurrently. * Updated README.md * Some Lint fixes * Updated tests to mock build_config_managers_from_yaml. * Fixed reference to directory. * Update README.md Co-authored-by: Neha Nene <[email protected]> * Update README.md Co-authored-by: Neha Nene <[email protected]> * Update README.md Co-authored-by: Neha Nene <[email protected]> * Update docs/internal/kubernetes_jobs.md Co-authored-by: Neha Nene <[email protected]> * Updated docs * Update README.md Co-authored-by: Neha Nene <[email protected]> * Some more doc changes. * Final changes ? * Final typos --------- Co-authored-by: Neha Nene <[email protected]>
The following command:
generates 164 partitioned configs as expected but the none of those can be run using
data-validation configs run -c ~/config_X.yaml
throwing the follow:It seems the
filter: <sql_where_clause>
part of the YALM contains timestamp values without the required quotes (see_creation_timestamp
below).The text was updated successfully, but these errors were encountered: