feat: Add integration tests for dbt import #5899
Conversation
YassinNouh21
force-pushed
the
copilot/add-integration-tests-for-dbt-import
branch
from
4423c32 to
37224b0
Compare
| dbt deps | ||
| dbt build | ||
|
|
||
| - name: Setup Feast project for dbt import test |
There was a problem hiding this comment.
Nice! Can you instead put most of the logic into the Makefile so that we can easily run make dbt-integration-test and swap that here?
|
|
||
| filterwarnings = | ||
| error::_pytest.warning_types.PytestConfigWarning | ||
| error::_pytest.warning_types.PytestUnhandledCoroutineWarning |
YassinNouh21
left a comment
YassinNouh21
left a comment
There was a problem hiding this comment.
Addressed review comments in the latest commit:
Comment 1 (Makefile): Created test-python-integration-dbt target that consolidates all the dbt test logic. Now you can run make test-python-integration-dbt locally!
Comment 2 (pytest.ini): The error::_pytest.warning_types.PytestUnhandledCoroutineWarning line was accidentally removed. It's been restored.
|
@YassinNouh21 can you post a screen shot of the feast UI and the dbt UI lineage graphs to see how they render? Also looks like you may have this hooked up to mcp to handle this, which is pretty cool |
|
@YassinNouh21 you can spin up the UI locally with |
|
|
hmm the protobuf issue sounds like a real concern, no? |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
⚠️ 1 issue in files not directly in the diff
⚠️ FEAST_TYPE_TO_VALUE_TYPE maps Int32 to ValueType.INT64 and Float32 to ValueType.DOUBLE instead of their correct narrower types (sdk/python/feast/dbt/mapper.py:32-35)
The FEAST_TYPE_TO_VALUE_TYPE mapping in the production code incorrectly widens Int32 → ValueType.INT64 and Float32 → ValueType.DOUBLE, losing precision information for entity value types.
Root Cause and Impact
ValueType.INT32 (value 3) and ValueType.FLOAT (value 6) both exist in feast/value_type.py:43-46, yet the mapping at sdk/python/feast/dbt/mapper.py:32 maps Int32 to ValueType.INT64 and at line 34 maps Float32 to ValueType.DOUBLE.
This mapping is used by _infer_entity_value_type (mapper.py:201-207) which is called during create_feature_view and create_all_from_model to set the entity's value_type. As a result, when a dbt model declares an entity column as int32 or float32, the created Feast Entity will have a widened value type (INT64 / DOUBLE), which may cause type mismatches downstream when the entity is used for online/offline lookups against stores that distinguish between 32-bit and 64-bit types.
Actual: Int32 → ValueType.INT64, Float32 → ValueType.DOUBLE
Expected: Int32 → ValueType.INT32, Float32 → ValueType.FLOAT
View 6 additional findings in Devin Review.
There was a problem hiding this comment.
Devin Review found 3 new potential issues.
⚠️ 2 issues in files not directly in the diff
⚠️ Codegen excludes entity columns from FeatureView schema, contradicting mapper's documented design (sdk/python/feast/dbt/codegen.py:251)
The DbtCodeGenerator.generate() method at sdk/python/feast/dbt/codegen.py:251 builds its exclusion set as excluded = set(entity_cols) | {self.timestamp_field}, which removes entity columns from the generated FeatureView's schema fields. This directly contradicts the mapper's behavior.
Root Cause: Inconsistent entity column handling between codegen and mapper
In sdk/python/feast/dbt/mapper.py:362-366, the mapper explicitly documents that entity columns should NOT be excluded from the schema:
# Note: entity columns should NOT be excluded - FeatureView.__init__
# expects entity columns to be in the schema and will extract them
excluded = {ts_field}But the codegen at sdk/python/feast/dbt/codegen.py:251 does the opposite:
excluded = set(entity_cols) | {self.timestamp_field}This means:
- Mapper API creates FeatureViews with entity columns in schema (e.g.,
driver_idappears infv.schema) - Codegen API (and CLI
--output) creates FeatureViews WITHOUT entity columns in schema
The test at test_dbt_integration.py:233-235 confirms the mapper keeps entity columns: assert "driver_id" in feature_names, but no codegen test verifies schema contents, so the discrepancy goes undetected.
Impact: FeatureViews created via generated code will have different schema content than those created via the mapper. This can cause inconsistencies when downstream code inspects fv.schema for data validation, column discovery, or schema evolution checks.
⚠️ FEAST_TYPE_TO_VALUE_TYPE maps Int32 to ValueType.INT64 and Float32 to ValueType.DOUBLE instead of their correct narrower types (sdk/python/feast/dbt/mapper.py:32-35)
The FEAST_TYPE_TO_VALUE_TYPE mapping in the production code incorrectly widens Int32 → ValueType.INT64 and Float32 → ValueType.DOUBLE, losing precision information for entity value types.
Root Cause and Impact
ValueType.INT32 (value 3) and ValueType.FLOAT (value 6) both exist in feast/value_type.py:43-46, yet the mapping at sdk/python/feast/dbt/mapper.py:32 maps Int32 to ValueType.INT64 and at line 34 maps Float32 to ValueType.DOUBLE.
This mapping is used by _infer_entity_value_type (mapper.py:201-207) which is called during create_feature_view and create_all_from_model to set the entity's value_type. As a result, when a dbt model declares an entity column as int32 or float32, the created Feast Entity will have a widened value type (INT64 / DOUBLE), which may cause type mismatches downstream when the entity is used for online/offline lookups against stores that distinguish between 32-bit and 64-bit types.
Actual: Int32 → ValueType.INT64, Float32 → ValueType.DOUBLE
Expected: Int32 → ValueType.INT32, Float32 → ValueType.FLOAT
View 14 additional findings in Devin Review.
| @cd sdk/python/tests/integration/dbt && \ | ||
| echo "Setting up Feast project..." && \ | ||
| mkdir -p feast_repo/data && \ | ||
| echo "project: feast_dbt_test\nregistry: data/registry.db\nprovider: local\nonline_store:\n type: sqlite\n path: data/online_store.db" > feast_repo/feature_store.yaml |
There was a problem hiding this comment.
🟡 Makefile echo with \n escape sequences relies on shell-specific behavior, fails on bash
The echo command on line 200 uses \n to produce newlines in the generated feature_store.yaml, but POSIX echo behavior with escape sequences is implementation-defined. On systems where /bin/sh is bash (or when Make's SHELL is set to bash), echo does NOT interpret \n, producing a single-line file with literal \n text — resulting in invalid YAML that causes the subsequent feast dbt import command to fail.
Root Cause and Impact
The command:
echo "project: feast_dbt_test\nregistry: data/registry.db\n..." > feast_repo/feature_store.yaml
On Ubuntu CI, /bin/sh is dash, which interprets \n in echo — so this works in CI. However, on macOS or systems where bash is the default shell, echo outputs literal \n, producing:
project: feast_dbt_test\nregistry: data/registry.db\nprovider: local\nonline_store:\n type: sqlite\n path: data/online_store.db
This is a single line of invalid YAML, causing feast dbt import on the next recipe line to fail with a YAML parsing error. Using printf instead of echo is the portable fix, since printf always interprets escape sequences.
Impact: make test-python-integration-dbt fails on developer machines running macOS or any system where Make's shell is bash.
| echo "project: feast_dbt_test\nregistry: data/registry.db\nprovider: local\nonline_store:\n type: sqlite\n path: data/online_store.db" > feast_repo/feature_store.yaml | |
| printf 'project: feast_dbt_test\nregistry: data/registry.db\nprovider: local\nonline_store:\n type: sqlite\n path: data/online_store.db\n' > feast_repo/feature_store.yaml |
Was this helpful? React with 👍 or 👎 to provide feedback.
YassinNouh21
force-pushed
the
copilot/add-integration-tests-for-dbt-import
branch
2 times, most recently
from
8b034ca to
4318134
Compare
|
DCO check failing due to unsigned commits. Suggest squash merge with Signed-off-by to resolve. |
|
@YassinNouh21 you can also click the DCO button in the UI |
YassinNouh21
force-pushed
the
copilot/add-integration-tests-for-dbt-import
branch
2 times, most recently
from
4896e07 to
e971bc1
Compare
- Add 82 integration tests covering dbt manifest parsing, mapper, codegen, and CLI - Add real dbt project with DuckDB for testing - Add GitHub Actions workflow for dbt integration tests - Fix entity column handling: codegen now includes entity columns in schema to match mapper behavior (FeatureView.__init__ expects them) - Fix type mapping: Int32→ValueType.INT32, Float32→ValueType.FLOAT (instead of widening to INT64/DOUBLE) - Add multi-entity column support tests - Add comprehensive test coverage for edge cases, validation errors, special characters, and CLI commands Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
YassinNouh21
force-pushed
the
copilot/add-integration-tests-for-dbt-import
branch
from
e971bc1 to
3b5a3f8
Compare
|
@franciscojavierarceo PR is now clean with all tests passing. Ready for review. |
|
|
||
| jobs: | ||
| dbt-integration-test: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
can we have a check for ok-to-test label so that resources don't get spin up for every PR raised, similar to other integration tests pattern
https://github.com/feast-dev/feast/blob/master/.github/workflows/pr_local_integration_tests.yml#L13
|
@ntkathole Done! Added the ok-to-test label check following the same pattern as pr_local_integration_tests.yml |
- Add PR trigger types (opened, synchronize, labeled) - Add label check condition requiring ok-to-test/approved/lgtm - Prevents resource spin-up for every PR Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
YassinNouh21
force-pushed
the
copilot/add-integration-tests-for-dbt-import
branch
from
1b9a0f4 to
e5209dd
Compare
3 participants
Summary
Test Coverage
Test plan