feat: Allow config and functions access in stream maps __alias__ expressions#3606
Open
KaruturiS wants to merge 5 commits into
Open
feat: Allow config and functions access in stream maps __alias__ expressions#3606KaruturiS wants to merge 5 commits into
__alias__ expressions#3606KaruturiS wants to merge 5 commits into
Conversation
Contributor
Reviewer's GuideRefactors stream alias evaluation to be instance-based so that alias expressions can access mapper configuration and a richer set of helper functions, adds tests for configuration- and function-driven aliasing, and tightens alias evaluation behavior and logging. Sequence diagram for instance-based stream alias evaluationsequenceDiagram
participant PM as PluginMapper
participant RRS as register_raw_stream_schema
participant ES as _eval_stream
participant ME as _MapperEval
PM->>RRS: register_raw_stream_schema(source_stream, stream_def)
RRS->>RRS: detect __alias__ in stream_def
RRS->>ES: _eval_stream(expr, stream_name)
ES->>ES: build names {__stream_name__, config=map_config}
ES->>ES: build functions from DEFAULT_FUNCTIONS + md5, sha256, datetime, bool, json
ES->>ME: create _MapperEval(names, functions)
ME-->>ES: instance
ES->>ME: eval(expr)
alt expression evaluates successfully
ME-->>ES: result (any type)
ES->>ES: cast result to str
ES-->>RRS: alias_str
else NameNotDefined
ME-->>ES: raise NameNotDefined
ES-->>RRS: original expr as str
else InvalidExpression or SyntaxError
ME-->>ES: raise error
ES-->>RRS: propagate MapExpressionError
end
RRS->>RRS: update stream_alias with evaluated value
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 1 issue, and left some high level feedback:
- In
_eval_stream, the comment mentionsself.functionsbut the evaluator is constructed with onlysimpleeval.DEFAULT_FUNCTIONS; ifPluginMappercan expose custom functions, consider wiring them in (e.g., viagetattr(self, "functions", simpleeval.DEFAULT_FUNCTIONS)) or updating the comment for consistency. - The new use of
self.map_configin_eval_streamassumes it is always defined; if there are code paths wheremap_configmay be absent orNone, consider a defensive default (e.g.,{}) to avoid unexpected attribute or type errors in alias expressions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_eval_stream`, the comment mentions `self.functions` but the evaluator is constructed with only `simpleeval.DEFAULT_FUNCTIONS`; if `PluginMapper` can expose custom functions, consider wiring them in (e.g., via `getattr(self, "functions", simpleeval.DEFAULT_FUNCTIONS)`) or updating the comment for consistency.
- The new use of `self.map_config` in `_eval_stream` assumes it is always defined; if there are code paths where `map_config` may be absent or `None`, consider a defensive default (e.g., `{}`) to avoid unexpected attribute or type errors in alias expressions.
## Individual Comments
### Comment 1
<location path="singer_sdk/mapper.py" line_range="867-877" />
<code_context>
try:
- expr_evaluator = simpleeval.EvalWithCompoundTypes(names=names)
+ # Access self.functions (inherited/available in PluginMapper)
+ # PluginMapper doesn't have a .functions property by default,
+ # so we'll use a safe fallback or the CustomStreamMap approach.
+
+ expr_evaluator = simpleeval.EvalWithCompoundTypes(
+ names=names,
+ functions=simpleeval.DEFAULT_FUNCTIONS
+ )
result = expr_evaluator.eval(expr)
</code_context>
<issue_to_address>
**suggestion:** Use mapper-specific functions instead of hardcoding `DEFAULT_FUNCTIONS`.
Here the evaluator is always initialized with `simpleeval.DEFAULT_FUNCTIONS`, so any mapper-specific functions (e.g., `self.functions` on `PluginMapper` subclasses) will never be used. Consider something like `functions=getattr(self, "functions", simpleeval.DEFAULT_FUNCTIONS)` so alias expressions can use custom helpers while still falling back to the safe default.
```suggestion
result: str
try:
# Use mapper-specific functions when available (e.g., on PluginMapper
# subclasses), falling back to the safe default set provided by
# simpleeval.DEFAULT_FUNCTIONS.
functions = getattr(self, "functions", simpleeval.DEFAULT_FUNCTIONS)
expr_evaluator = simpleeval.EvalWithCompoundTypes(
names=names,
functions=functions,
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Documentation build overview
13 files changed ·
|
… HTTP request for a stream (meltano#3483) ## Related - meltano#1606 - MeltanoLabs/tap-dbt#135 - PoC: reservoir-data/tap-bitly#494 ## Summary by Sourcery Introduce a dedicated HTTPRequest abstraction in the REST stream layer and update tap-dummyjson to use it for request construction and parameter handling. New Features: - Add an HTTPRequest dataclass for REST streams, including customizable query parameter encoding and reusable request construction via RESTStream.get_http_request. Enhancements: - Refactor RESTStream.prepare_request to build requests from HTTPRequest instances and standardize URL parameter typing and encoding. - Simplify DummyJSONAuthenticator to always set the Authorization header after ensuring a valid token. - Update tap-dummyjson client pagination to override get_http_request instead of get_url_params for setting paging parameters. Tests: - Add a unit test covering custom HTTPRequest.encode_params behavior with safe characters in query parameters. ## Summary by Sourcery Introduce an HTTPRequest abstraction and context for RESTStream to construct and customize HTTP requests, and migrate pagination and client patterns to use it instead of get_url_params while deprecating prepare_request overrides. New Features: - Add an HTTPRequest dataclass and HTTPRequestContext for representing REST stream HTTP requests and their pagination/context. - Expose RESTStream.get_http_request for building per-request URL, headers, params, and payload from stream context and paginator state. Enhancements: - Refactor RESTStream request preparation to build PreparedRequest objects from HTTPRequest instances with standardized payload typing and parameter encoding. - Update built-in taps (dummyjson, GitLab) and the cookiecutter tap template to implement pagination by overriding get_http_request instead of get_url_params. - Improve paginator typing for RESTStream and BaseAPIPaginator implementations. - Emit deprecation warnings when prepare_request is overridden, including when the override is in an intermediate base class. - Adjust documentation examples to show customizing pagination by modifying HTTPRequest rather than URL parameter dicts. Documentation: - Add reference stubs for HTTPRequest and HTTPRequestContext and update pagination and incremental replication guides to document get_http_request-based pagination patterns. Tests: - Add unit tests for HTTPRequest parameter encoding, including custom safe characters and non-string parameter values. - Extend REST pagination and metrics tests to cover the new get_http_request usage and deprecation warnings for prepare_request overrides. --------- Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
edgarrmondragon
force-pushed
the
feat/issue-3263-alias-functions
branch
from
7038c43 to
7163691
Compare
edgarrmondragon
changed the title
__alias__ expressions
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3606 +/- ##
=======================================
Coverage 93.85% 93.86%
=======================================
Files 73 73
Lines 5907 5946 +39
Branches 725 729 +4
=======================================
+ Hits 5544 5581 +37
- Misses 270 271 +1
- Partials 93 94 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Collaborator
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider avoiding re-creating the
funcsdictionary on every_eval_streamcall by promoting it to a shared constant or helper so alias evaluation reuses the same function set and avoids per-call allocations. - Since
_eval_streamnow relies onself.map_config, it may be worth ensuring this is always a dictionary (or defaulting to{}) to avoid surprises if plugins constructPluginMapperwithoutstream_map_config.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding re-creating the `funcs` dictionary on every `_eval_stream` call by promoting it to a shared constant or helper so alias evaluation reuses the same function set and avoids per-call allocations.
- Since `_eval_stream` now relies on `self.map_config`, it may be worth ensuring this is always a dictionary (or defaulting to `{}`) to avoid surprises if plugins construct `PluginMapper` without `stream_map_config`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
2 participants
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.
This PR refactors _eval_stream from a static method to an instance method, allowing stream aliases to access self.map_config and functions. This enables dynamic aliasing based on user configuration.
Summary by Sourcery
Allow stream alias expressions to be evaluated with mapper instance context, including configuration and helper functions.
New Features:
Tests: