-
Notifications
You must be signed in to change notification settings - Fork 42
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: Data source cache #2164
feat: Data source cache #2164
Conversation
/// | ||
/// This means we have ~3600 OIDs to play with for builtin objects. Note that | ||
/// once a builtin object is given a stable OID, it **must not** be changed ever | ||
/// (unless you're the person willing to write a migration system). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which we should do in a while?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely, just want to try to delay it as much as possible to better understand what needs to happen for a system like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1000% agree.
/// First glaredb builtin OID: 16384 | ||
/// First user object OID: 20000 | ||
/// | ||
/// This means we have ~3600 OIDs to play with for builtin objects. Note that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do they have to be sequentially grouped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't, it's just easier to track what the next OID should be when adding a table/schema/whatever. If we have more than 16 builtin schemas (unlikely), we can just jump forward to some other available OID.
What's the relationship of this pr to #2201 |
I expect Cloud to use the simple query interface to call the cache function(s) in this PR for updating the cache, but they're two independent features. |
crates/datafusion_ext/src/system.rs
Outdated
/// | ||
/// This should be focused on operations that do not require user interactions | ||
/// (e.g. background operations like optimizing tables or running caching jobs). | ||
pub trait SystemOperation: Sync + Send { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we ever have an unknown number of SystemOperation
's, or custom operations? Considering we are dealing with an exhaustive list of operations, I'm wondering if it'd make sense to use an enum here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, will change
impl ConstBuiltinFunction for CacheExternalDatabaseTables { | ||
const NAME: &'static str = "cache_external_database_tables"; | ||
const DESCRIPTION: &'static str = "Cache tables from external databases."; | ||
const EXAMPLE: &'static str = "select * from cache_external_database_tables();"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something for another day, but it'd be cool to support the CALL
syntax for this.
while valid, select * from cache_external_database_tables();
seems weird to me as im selecting from an action.
CALL
seems fitting for something that is just executing a function call.
CALL cache_external_database_tables();`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I originally wanted to use CALL, but sqlparser doesn't support it.
Force merging, CI failure due to #2168 |
Adds caching of external database table info.