fix: #1572 - make where_clause_suffix an actual suffix

[delwaterman]

Signed-off-by: Orion Delwaterman [email protected]

Summary of Changes

Fixes #1572

Changes the config of RedshiftMetadataExtractor where_clause_suffix to not require a user to include a WHERE statement in their sql partial. This follows the behavior of PostgresMetadataExtractor, and seems more consistent with the variable name where_clause_suffix by treating the partial as a suffix to a where clause.

This will be a breaking change for anyone using redshift extractor (so probably will need a minor version bump). That being, IMHO, it's better to make the behavior follow expected patterns rather than ask users to hunt through source code to determine why their query is breaking (which happened to me).

Tests

databuilder/tests/unit/extractor/test_redshift_metadata_extractor.py

Documentation

CheckList

Make sure you have checked all steps below to ensure a timely review.

• PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
• PR includes a summary of changes.
• PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public functions and the classes in the PR contain docstrings that explain what it does

[dkunitsk]

Redshift extractor is reasonably popular and the style fix isn't worth breaking ingestors, imho. where True is always a workaround.

[delwaterman]

If I may, I lost 1 hour of development tracking down this issue, which doesn't feel like a lot, but forcing a user to dig through source code to understand how to use a public API is bad form. Personally it led me to trust this code base less, because it felt sloppy. I am not trying to pick a fight, but I am trying to tell my experience as a new developer using the project

That being said, why don't we do a comprimise, and support the old pattern, with a deprecation warning:

if where_clause_suffix.lower().startswith("where"):
    logger.Warn("you no longer need to begin with 'where' in your suffix")
    where_clause = where_clause_suffix
else:
    where_clause = f"where {where_clause_suffix}"

[verdan]

This is kind of an issue we had for Postgres, and we fixed it here: 6cf8965#diff-01ee0656007972e26ae47d9003cf877aefa524b6e91a6bf6b72035892ba9b757R47

So another suggestion is to make the where_clause_suffix a real suffix, what @delwaterman is suggesting, and make the default value as true.

But your's approach to supporting existing installation is also good. IMHO, announcing such breaking changes properly should not hurt the existing users. It's a minor change but will make life easy for any new installation.

[dkunitsk]

+1 to either of the nonbreaking alternatives mentioned above.

[delwaterman]

Ok I will do the update

[delwaterman]

@verdan @dkunitsk This is now ready

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[delwaterman]

Bump

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[delwaterman]

bump