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

Add DB_Table feature flags #11163

Merged
merged 59 commits into from
Oct 7, 2024
Merged

Add DB_Table feature flags #11163

merged 59 commits into from
Oct 7, 2024

Conversation

AdRiley
Copy link
Member

@AdRiley AdRiley commented Sep 25, 2024

Pull Request Description

This MR:

  • Introduces the framework for feature flags for the In Database processing engine
  • For everything other than SQLServer continues to have the same functionality and tests that they did before
  • For SQLServer removes all In-DB functionality other than being able to read a table and provides user friendly errors messages for all other In database funtionality

Important Notes

Note the current set of flags may not be the final set and some of the flagging out of tests is probably coarser than it will eventually be. As we work through SQLServer enbaling functionality and tests we will clean these up.

Next steps after this MR will be:

  • Decide on an order to approach adding SQLServer features
  • Enable flags for SQLServer one at a time and make the corresponding tests pass
  • I imagines we will need to extend the flags to Column level functionality
  • Look if we can swap some of the existing DB test flags for the new Feature flag system

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@AdRiley AdRiley marked this pull request as ready for review October 3, 2024 08:20
Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Like the approach puts the responsibility in correct place.

Approving but have left some suggested changes.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks great.

I especially appreciate the tests that ensure that if a feature is unsupported, it raises an informative error instead of crashing/returning garbage - these seem really useful.

I imagine we will need to increase granularity of the feature flags in next iterations, but that looks like a great starting point.

Only thing I don't really agree with is disabling DB_Table.at - IMO it should still be allowed, just operations on columns should raise an error. I should still be able to do my_table.at "my_column" . to_vector in basically any backend that supports 'Read table' feature. But open to being convinced otherwise :)

@AdRiley AdRiley merged commit d0f0770 into develop Oct 7, 2024
36 checks passed
@AdRiley AdRiley deleted the wip/adr/add-db-feature-flags branch October 7, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants