Add support for 'when' field in redis.lookup block#397
Add support for 'when' field in redis.lookup block#397ZdravkoDonev-redis wants to merge 1 commit into
Conversation
In order to reduce latency, we can avoid doing lookups altogether by looking in some of the fields in the input data and deciding whether we should doo a lookup in advance. Thus with this commit, a 'when' field is introduced to support this functionality
There was a problem hiding this comment.
Pull request overview
This PR adds conditional execution support to the redis.lookup block through a new when field, allowing lookups to be skipped based on input data conditions to reduce latency.
Key Changes:
- Added
whenconfiguration field supporting JMESPath and SQL expressions - Modified lookup execution logic to evaluate conditions and skip lookups when conditions are false
- Added comprehensive integration tests for both JMESPath and SQL condition languages
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
core/src/datayoga_core/blocks/redis/lookup/block.py |
Implements conditional lookup logic with when expression compilation and evaluation |
core/src/datayoga_core/blocks/redis/lookup/block.schema.json |
Adds schema definition for the optional when field with expression and language properties |
integration-tests/test_redis_lookup.py |
Adds parameterized test validating when condition functionality for both expression languages |
integration-tests/resources/jobs/tests/redis_lookup_with_when_jmespath.dy.yaml |
Job configuration demonstrating JMESPath when condition usage |
integration-tests/resources/jobs/tests/redis_lookup_with_when_sql.dy.yaml |
Job configuration demonstrating SQL when condition usage |
integration-tests/resources/data/lookup_with_nulls.csv |
Test data file with records having null/empty branch_id values for condition testing |
core/pyproject.toml |
Adds redis and testcontainers as dev dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "job_name", | ||
| [("tests.redis_lookup_with_when_jmespath"), | ||
| ("tests.redis_lookup_with_when_sql")]) | ||
| def test_redis_lookup_with_when_condition(job_name: str): |
There was a problem hiding this comment.
The test function lacks coverage for error handling scenarios when the when condition evaluation fails. Consider adding test cases for malformed expressions or invalid condition syntax to ensure the error handling in block.py (lines 61-63) works correctly.
| 3,set_key,branch_3 | ||
| 4,sorted_set_key,branch_4 | ||
| 5,list_key, | ||
|
|
There was a problem hiding this comment.
The CSV file contains an empty line at the end which may cause unexpected behavior during parsing. Remove the trailing empty line to ensure consistent data processing.
| condition_result = self.when_expression.search(record) | ||
| should_lookup = bool(condition_result) | ||
| if not should_lookup: | ||
| logger.debug(f"Skipping lookup for record {idx} due to when condition") |
There was a problem hiding this comment.
The debug message could be more informative by including the condition result or expression details to aid in troubleshooting. Consider adding the evaluated condition value to the log message.
| logger.debug(f"Skipping lookup for record {idx} due to when condition") | |
| logger.debug(f"Skipping lookup for record {idx} due to when condition evaluated as {condition_result!r}") |
spicy-sauce
left a comment
spicy-sauce
left a comment
There was a problem hiding this comment.
Hi Zdravko,
The CI is failing partly because this PR has main as both the head and base branch, which prevents the auto-commit workflows (generate-docs, generate-jsonschemas) from pushing changes back.
Could you push your changes to a feature branch (e.g., add-redis-lookup-when-field) and create a new PR from there to main?
Note: There's also a separate CI issue with a pymssql/setuptools_scm dependency conflict that we'll need to fix on our end.
Hi, Yossi! :) Sure I'll do that today. And will try to resolve the copilot comments as well. |
|
Closed in favor of - #398 |
|
@spicy-sauce Please have a look at the new PR - #398 |
In order to reduce latency, we can avoid doing lookups altogether by looking in some of the fields in the input data and deciding whether we should doo a lookup in advance. Thus with this commit, a 'when' field is introduced to support this functionality