fix(targets): honor 'schema' setting as alias for default_target_schema#3689
Open
Rasaec5 wants to merge 15 commits into
Open
fix(targets): honor 'schema' setting as alias for default_target_schema#3689Rasaec5 wants to merge 15 commits into
Rasaec5 wants to merge 15 commits into
Conversation
Added a section to CW_README explaining the choice of issue meltano#2766 and personal qualifications for tackling it.
Expanded the CW_README to include steps to reproduce the issue, reproduction evidence, an implementation plan, and details on supporting changes and tests.
Update CW_README to clarify the solution plan and schema handling for SDK SQL targets.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Added schema_config property to retrieve schema from config.
Add parameterized test for schema name settings in DummySQLSink.
Contributor
Reviewer's GuideHonor the Sequence diagram for SQLSink schema_name resolution ordersequenceDiagram
participant SQLTarget
participant SQLSink
participant Config
participant Connector
SQLTarget->>SQLSink: full_table_name()
SQLSink->>SQLSink: table_name
SQLSink->>SQLSink: schema_name
SQLSink->>Config: get(default_target_schema)
alt default_target_schema set
SQLSink-->>SQLSink: use default_target_schema
else default_target_schema not set
SQLSink->>Config: get(schema)
alt schema set
SQLSink-->>SQLSink: use schema_config
else schema not set
SQLSink-->>SQLSink: derive schema from stream_name
end
end
SQLSink->>Connector: get_fully_qualified_name(table_name,schema_name,database_name)
Connector-->>SQLTarget: FullyQualifiedName
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Documentation build overview
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
Contribution_README.mdfile appears to be contributor notes rather than project code; consider removing it from the PR or moving this content to an appropriate project documentation location if desired. - The new
schemaalias behavior is tested both intests/sql/test_sink.pyand in the dedicatedtests/sql/test_schema_alias.py; consider consolidating these into a single suite to avoid duplication and keep the behavior in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Contribution_README.md` file appears to be contributor notes rather than project code; consider removing it from the PR or moving this content to an appropriate project documentation location if desired.
- The new `schema` alias behavior is tested both in `tests/sql/test_sink.py` and in the dedicated `tests/sql/test_schema_alias.py`; consider consolidating these into a single suite to avoid duplication and keep the behavior in one place.
## Individual Comments
### Comment 1
<location path="tests/sql/test_schema_alias.py" line_range="84-66" />
<code_context>
+ "target_schema",
+ id="schema-used-for-single-part-stream",
+ ),
+ # default_target_schema always wins when both are set.
+ pytest.param(
+ "TAP_SCHEMA-users",
+ {"schema": "ignored", "default_target_schema": "default_schema"},
</code_context>
<issue_to_address>
**suggestion (testing):** Add a case for an empty-string `default_target_schema` to confirm it does not override `schema`.
Because `default_target_schema` is treated as truthy/falsy, an empty string currently behaves like “not set” and falls back to `schema_config` and then stream-derived logic. Please add a parametrized test with `default_target_schema: ""` and `schema: "target_schema"` asserting that `schema` still wins, to protect against future changes that might treat empty strings as a valid override.
</issue_to_address>
### Comment 2
<location path="Contribution_README.md" line_range="57" />
<code_context>
+
+1. **Decide on `schema` precedence vs stream-derived.** As above, an explicit `schema` setting
+ should win over the implicit stream prefix — that is the user's intent in the bug report.
+1. **Builtin setting (optional).** Consider registering `schema` (and/or treating it as an alias
+ of `default_target_schema`) in `SQLTarget`'s base config schema so all SQL targets get it
+ consistently, rather than relying on each target to define it.
</code_context>
<issue_to_address>
**issue (typo):** Use the standard hyphenated form "Built-in" instead of "Builtin".
Here, "built-in" is used as an adjective, which is conventionally hyphenated. Updating "Builtin" to "Built-in" will match standard usage.
```suggestion
1. **Built-in setting (optional).** Consider registering `schema` (and/or treating it as an alias
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "stream_name, config, expected_schema_name", | ||
| [ | ||
| # `schema` is honored when default_target_schema is absent. | ||
| pytest.param( |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a case for an empty-string default_target_schema to confirm it does not override schema.
Because default_target_schema is treated as truthy/falsy, an empty string currently behaves like “not set” and falls back to schema_config and then stream-derived logic. Please add a parametrized test with default_target_schema: "" and schema: "target_schema" asserting that schema still wins, to protect against future changes that might treat empty strings as a valid override.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3689 +/- ##
=======================================
Coverage 94.21% 94.22%
=======================================
Files 73 73
Lines 6208 6213 +5
Branches 763 764 +1
=======================================
+ Hits 5849 5854 +5
Misses 267 267
Partials 92 92
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
When a SQL target sets a schema setting but does not set default_target_schema,
the SDK now uses the configured schema as the destination schema instead of deriving
one from the incoming stream ID.
Why
Closes #2766.
Many SQL targets expose a schema connection setting (for example the MeltanoLabs
variant of target-snowflake). Today, if default_target_schema is unset,
SQLSink.schema_name derives the schema from the stream ID — e.g. a record on stream
TAP_SCHEMA-users is written to a schema named tap_schema. When the connecting
account has no privileges on that derived schema, schema/table/file-format creation
fails. Users reasonably expect the schema they configured to be honored.
How
SQLSink.schema_name now resolves in this order:
default_target_schema (unchanged — keeps precedence so existing setups are unaffected)
/ --schema (new fallback)
stream-derived (-
None
Implementation:
Added a SQLSink.schema_config cached property that reads the schema setting and
returns it verbatim (no name conforming), mirroring the existing
default_target_schema property.
schema_name falls back to schema_config before the stream-derived branch.
This is intentionally non-breaking: default_target_schema still wins when both are set,
and behavior is unchanged when neither is set.
Files changed
singer_sdk/sql/sink.py — add schema_config; update schema_name.
tests/sql/test_sink.py — add test_schema_name_schema_setting parametrized cases.
tests/sql/test_schema_alias.py — new dedicated test module for the alias behavior.
Testing
Unit tests run without external credentials using a dummy SQL connector/sink/target.
Resolution-order matrix: schema overrides stream-derived for 1-, 2-, and 3-part
stream names; default_target_schema takes precedence when both are set; only
default_target_schema set; neither set (stream-derived fallback, including None
for 1- and 4-part stream names).
Edge cases: empty-string schema falls through; schema returned verbatim;
table_name derivation unaffected.
Regression: existing default_target_schema / stream-derived cases still pass.
python -m pytest tests/sql/test_schema_alias.py tests/sql/test_sink.py -v
tests/sql/test_sink.py -> 13 passed
tests/sql/test_schema_alias.py -> 13 passed
Open question for reviewers
Should schema also be registered as a documented builtin setting on SQLTarget
(an explicit alias of default_target_schema), or is reading it from config enough?
This PR keeps it read-from-config to avoid colliding with targets that already define
their own schema setting.
Notes / checklist
X I have read the Contributing Guide.
X pre-commit run --all, nox -s tests, and nox -t typing pass locally.
X This contribution was assisted by an AI coding agent (per Meltano's Agentic Coding guidelines).
Summary by Sourcery
Honor the
schemaconfiguration as a fallback destination schema for SQL sinks whendefault_target_schemais not set, while preserving existing precedence and stream-derived behavior.New Features:
schemaconfig setting as an alias/fallback fordefault_target_schemain SQLSink schema resolution.Enhancements:
Documentation:
Tests:
default_target_schema,schema, and stream-derived names, including edge cases and regression coverage.