Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(taps): add ability to do list comprehensions in stream map expressions #2003

Merged
merged 7 commits into from
Oct 10, 2023

Conversation

haleemur
Copy link
Contributor

@haleemur haleemur commented Oct 7, 2023

Closes #2002

  • add simpleeval as a dependency & remove locally vendored simpleeval (since simpleeval itself moved to poetry and is no longer dependent on deprecated packages https://gitlab.com/meltano/sdk/-/issues/213)
  • use ast to parse expressions used for filtering and property mapping in stream_maps.
  • use simpleeval.EvalWithCompoundTypes instead of simpleEval.SimpleEval
  • add bool as a new transformed type for mapped properties
  • add test for list comprehension in mapped properties.
  • update tap test factory to check against final record schema (after stream maps are applied)
  • add a performance test through feat(taps): add ability to do list comprehensions in stream map expressions #2003

📚 Documentation preview 📚: https://meltano-sdk--2003.org.readthedocs.build/en/2003/

@edgarrmondragon
Copy link
Collaborator

edgarrmondragon commented Oct 7, 2023

Hey @haleemur,

Thanks for contributing! Ping me when this is ready for review 🙂

@tayloramurphy
Copy link
Collaborator

Thanks for the contribution @haleemur ! @edgarrmondragon can you open a separate issue for adding this as an example to https://sdk.meltano.com/en/latest/stream_maps.html ?

poetry.lock Outdated Show resolved Hide resolved
tests/core/test_simpleeval.py Outdated Show resolved Hide resolved
@haleemur haleemur force-pushed the simpleeval-compound-data-types branch from 0495ef0 to 22ea165 Compare October 10, 2023 21:04
@haleemur haleemur changed the title draft: add ability to do list comprehensions in stream map expressions feat(taps): add ability to do list comprehensions in stream map expressions Oct 10, 2023
@haleemur
Copy link
Contributor Author

@edgarrmondragon

I'll take a look at why mypy is complaining on 3.8, but otherwise, I think the PR is ready.

When looking at the benchmark run in the workflows a significant improvement is noted (likely due to parsing only once)

Locally run benchmarks reveal a similar story.

meltano:main locally run benchmark

------------------------------------------------------- benchmark: 1 tests ------------------------------------------------------
Name (time in ms)                         Min       Max      Mean  StdDev    Median     IQR  Outliers     OPS  Rounds  Iterations
---------------------------------------------------------------------------------------------------------------------------------
test_bench_simple_map_transforms     318.3243  320.8534  319.9556  0.9875  320.2287  1.1652       1;0  3.1254       5           1
---------------------------------------------------------------------------------------------------------------------------------

Legend:
  Outliers: 1 Standard Deviation from Mean; 1.5 IQR (InterQuartile Range) from 1st Quartile and 3rd Quartile.
  OPS: Operations Per Second, computed as 1 / Mean
============================================================= 1 passed, 473 skipped, 2 deselected, 8 warnings in 3.55s =============================================================

haleemur:simpleeval-compound-data-types locally run benchmark

----------------------------------------------------- benchmark: 1 tests -----------------------------------------------------
Name (time in ms)                        Min      Max     Mean  StdDev   Median     IQR  Outliers      OPS  Rounds  Iterations
------------------------------------------------------------------------------------------------------------------------------
test_bench_simple_map_transforms     84.7507  86.4868  85.4988  0.5254  85.5117  0.7484       3;0  11.6961      12           1
------------------------------------------------------------------------------------------------------------------------------

Legend:
  Outliers: 1 Standard Deviation from Mean; 1.5 IQR (InterQuartile Range) from 1st Quartile and 3rd Quartile.
  OPS: Operations Per Second, computed as 1 / Mean
============================================================= 1 passed, 390 skipped, 2 deselected, 8 warnings in 2.48s =============================================================

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #2003 (fd56bb2) into main (7675942) will decrease coverage by 0.59%.
The diff coverage is 87.87%.

@@            Coverage Diff             @@
##             main    #2003      +/-   ##
==========================================
- Coverage   87.42%   86.83%   -0.59%     
==========================================
  Files          59       58       -1     
  Lines        5136     4884     -252     
  Branches      830      777      -53     
==========================================
- Hits         4490     4241     -249     
- Misses        451      456       +5     
+ Partials      195      187       -8     
Files Coverage Δ
singer_sdk/testing/factory.py 93.85% <100.00%> (+0.05%) ⬆️
singer_sdk/testing/tap_tests.py 84.49% <100.00%> (ø)
singer_sdk/mapper.py 79.76% <84.00%> (-0.91%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@edgarrmondragon edgarrmondragon self-requested a review October 10, 2023 22:56
Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing. Thanks @haleemur!

@edgarrmondragon edgarrmondragon added this pull request to the merge queue Oct 10, 2023
@edgarrmondragon edgarrmondragon removed this pull request from the merge queue due to a manual request Oct 10, 2023
@edgarrmondragon edgarrmondragon added this pull request to the merge queue Oct 10, 2023
Merged via the queue into meltano:main with commit d00ba6a Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

feat: add ability to perform comprehensions in stream maps
3 participants