← 输入框|github.com|刷新
Skip to content

fix: Keep ConnectionRef tags out of DataSource tags when reading protos - #7068

Open
raams95 wants to merge 1 commit into
feast-dev:masterfrom
raams95:fix/data-source-connection-ref-tags
Open

raams95 wants to merge 1 commit into
feast-dev:masterfrom
raams95:fix/data-source-connection-ref-tags

Conversation

@raams95

@raams95 raams95 commented Oct 24, 2026

Copy link
Copy Markdown

What this PR does / why we need it:

DataSource.to_proto() writes a connection_ref into the proto tags as feast.connection-ref.* entries. DataSource.from_proto() rebuilds connection_ref from those entries, but it also leaves them in DataSource.tags. That causes two problems:

  1. A source with a connection_ref no longer equals itself after a registry round trip, because tags now has the extra keys.

    src = FileSource(name="s", path="x.parquet", timestamp_field="ts",
                     tags={"team": "ml"},
                     connection_ref=ConnectionRef(provider="env", name="DRIVER"))
    restored = DataSource.from_proto(src.to_proto())
    restored.tags   # {'team': 'ml', 'feast.connection-ref.provider': 'env', 'feast.connection-ref.name': 'DRIVER'}
    restored == src  # False

    This shows up when pinning a feature view version. If the feature view's source has a connection_ref, applying it with version="v0" and no other change raises FeatureViewPinConflict, because apply_feature_view compares the user's view with the stored one and the batch sources differ only in those tags.

  2. Changing connection_ref on a loaded source doesn't stick. The old entries are still in tags, so the next to_proto() writes them back. Setting connection_ref = None brings the old reference back on the next load. Replacing it with a new ConnectionRef keeps the old params.

The fix drops the feast.connection-ref.* keys from tags in DataSource.from_proto(), right after connection_ref is rebuilt from them. Everything else stays as it is. The proto still carries the tags, so stored registries and the proto diff in feast plan are unchanged.

Which issue(s) this PR fixes:

No existing issue. I found this while reading the ConnectionRef code and can open one if you'd like.

Checks

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
  • Integration tests
  • Manual tests
  • Testing is not required for this change

Added two tests to sdk/python/tests/unit/test_data_sources.py. One checks that a round trip keeps tags and connection_ref and that the source still equals the original. The other checks that clearing or replacing connection_ref on a loaded source survives another round trip. Both fail on master and pass with this change. I also ran test_credentials.py, test_feature_views.py, test_feature_view_versioning.py, test_stream_feature_view.py, test_unit_feature_store.py, local_feast_tests, infra/registry, infra/data_sources, infra/offline_stores and the iceberg_catalog tests. They show the same results with and without the change. The only failures in either run come from optional dependencies I don't have installed locally (pyspark) or from tests that need network access. ruff check, ruff format --check and mypy pass on the changed files.

Misc

DataSource.to_proto() stores a ConnectionRef as feast.connection-ref.*
entries in the proto tags. from_proto() rebuilt connection_ref from those
entries but also left them in DataSource.tags, so a source with a
connection_ref no longer compared equal to itself after a registry round
trip, and clearing or replacing connection_ref on a loaded source had no
effect because the stale tags were written back on the next to_proto().

One visible effect: pinning a feature view whose source has a
connection_ref (version="v0") raised FeatureViewPinConflict even when the
definition was unchanged.

Signed-off-by: JanakiRamChittineni <341781521+raams95@users.noreply.github.com>
@raams95
raams95 requested a review from a team as a code owner October 24, 2026 02:13

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant