Skip to content

Chore!: Replace strings with SamplerType enum#2565

Merged
mivds merged 3 commits intomainfrom
DTL-1623/sampler-type-enum
Feb 5, 2026
Merged

Chore!: Replace strings with SamplerType enum#2565
mivds merged 3 commits intomainfrom
DTL-1623/sampler-type-enum

Conversation

@mivds
Copy link
Contributor

@mivds mivds commented Feb 4, 2026 •
edited
Loading

Description

Please provide a description of your changes:

  • What problem are you solving?
    • Replace magic strings with an enum to avoid breakage
    • Avoid requirement for string values to match between Soda Cloud & Core (there is now a mapping function)
  • Any expected impact on downstream packages/services?
    • Yes, soda-extensions will need to use the SamplerType enum instead of strings. The only practical impact is in tests. It is not used in the packages anywhere.
  • Reference issue # if available.

Checklist

@mivds mivds self-assigned this Feb 4, 2026
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 4, 2026

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@mivds mivds marked this pull request as ready for review February 4, 2026 10:32
@mivds mivds requested review from Niels-b and m1n0 February 4, 2026 10:32
@mivds mivds changed the title Chore: Replace strings with SamplerType enum Chore!: Replace strings with SamplerType enum Feb 5, 2026
m1n0
m1n0 approved these changes Feb 5, 2026
View reviewed changes
Copy link
Contributor

@m1n0 m1n0 left a comment

Choose a reason for hiding this comment

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

LGTM, out of curiosity - is the motivation to separate internal from cloud types even though they match just for the sake of separation or do you have anything else in mind?

@mivds
Copy link
Contributor Author

mivds commented Feb 5, 2026

LGTM, out of curiosity - is the motivation to separate internal from cloud types even though they match just for the sake of separation or do you have anything else in mind?

Separation only really. When working with the sampling functionality I didn't want to use the magic string "absoluteLimit", since that is very susceptible to breaking. I wanted to introduce some form of typing. Then there was the choice of using the existing SamplerType defined by cloud, or introduce a new enum.

Since the sampling is part of our SQL AST, I figured it was best to introduce a new type rather than create a dependency from AST on cloud types. We've talked about extracting the AST in a separate package before; those kinds of dependencies would block that.

@mivds mivds merged commit 1c44965 into main Feb 5, 2026
41 checks passed
@mivds mivds deleted the DTL-1623/sampler-type-enum branch February 5, 2026 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@m1n0 m1n0 m1n0 approved these changes

@Niels-b Niels-b Awaiting requested review from Niels-b

Assignees

@mivds mivds

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mivds @m1n0