Skip to content
New issue

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

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

Already on GitHub? # 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
25 of 26 checks passed
# for free to join this conversation on GitHub. Already have an account? # 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