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

Split use_asyncio from use_smartnoise_sql and make the latter query specific #125

Merged
merged 4 commits into from
Jul 21, 2023

Conversation

mhauru
Copy link
Collaborator

@mhauru mhauru commented Jul 20, 2023

Two new features:

  • use_asyncio and use_smartnoise_sql are now separate configuration parameters, with the rule that both can't be true at the same time (a limitation of snsql)
  • use_smartnoise_sql is no longer a global option, but set individually for each src-stats query.

Joint work with @Olajumoke01.

@mhauru mhauru requested a review from Iain-S July 20, 2023 16:02
Copy link
Collaborator

@Iain-S Iain-S left a comment

Choose a reason for hiding this comment

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

LGTM. Tested with MariaDB and this allowed me to use SRC_STATS as long as use-asyncio is false.

@Iain-S
Copy link
Collaborator

Iain-S commented Jul 21, 2023

Quick thought, would it be better to have the block be

smartnoise:
  epsilon: 1
  delta: 0.1

rather than

use-smartnoise: true
epsilon: 1
delta: 0.1

since it doesn't make sense to declare epsilon and delta unless we are using smartnoise?
Leaving out (or commenting out) the whole smartnoise block would then turn it off.

@mhauru
Copy link
Collaborator Author

mhauru commented Jul 21, 2023

That makes sense, but this will soon change again when we introduce the two-stage src-stats (first a non-DP DB query, then a follow up query on the dataframe locally with DP), so I'm inclined to leave it as is now. The other PR is already in the works.

@mhauru mhauru merged commit 04b3641 into main Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants