fix: Row validations fail for values with trailing newline #1415
Conversation
…ns-fail-for-values-with-trailing-newline
|
/gcbrun |
There was a problem hiding this comment.
Looks great!
To address one of your questions in the comments... I believe we originally added the rstrip to account for the teradatasql bug that adds extra padding when returning results. For validations that don't include TD as a source or target we probably don't need the rstrip as a sanitization step before hashing but it's good that this PR now fixes the whitespace bug 😄
There was a problem hiding this comment.
Nick,
Thank you for the work on this. I understand this is holding up a customer - so will approve this.
I agree with Neha that we are overusing RTRIM/Rstrip and distorting validation results. I have opened a separate issue requesting this be fixed.
Thanks again for the hard work and the test cases!!
Sundar Mudupallli
…-trailing-newline
Co-authored-by: Helen Cristina <[email protected]>
Co-authored-by: Helen Cristina <[email protected]>
|
/gcbrun |
There was a problem hiding this comment.
Regarding the IF EXISTS part, I definitely was unsure of it about Teradata and for Oracle I did some search before and I found it available, but I checked again and it's specific to version 23 apparently, so my bad!
I'll look further on proper ways to do this type of checking for TD and Oracle in the draft PR for the DDLs: #1416
nj1973
deleted the
1411-row-validations-fail-for-values-with-trailing-newline
branch
This PR merged changes to align RStrip behaviour across as many SQL engines as possible. The engines changed here are:
BigQuery, PostgreSQL, Spanner and Teradata already behaved correctly.
MySQL and Hive rstrip() functions do not support characters other than space and therefore are not included in this PR.
SQL Server has mixed support for customizing the characters to remove and I've created a specific issue for this: #1419
I've added integration tests for all systems above.