-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Lazy system tables #4606
Lazy system tables #4606
Conversation
} | ||
} | ||
|
||
impl PartitionStream for InformationSchemaTables { |
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.
This is significantly less verbose than having to define a TableProvider
and then a corresponding ExecutionPlan
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.
but more verbose than creating a MemTable
, right?
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.
Yeah, it was the best I could do
/// Construct the `information_schema.tables` virtual table | ||
fn make_tables(&self) -> Arc<dyn TableProvider> { | ||
fn make_tables(&self, builder: &mut InformationSchemaTablesBuilder) { |
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.
The signature of these is now a little bit strange, but I wanted to try to minimise code movement
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.
maybe we can refactor them in a follow on PR
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.
So the rationale for this PR is to defer the information table construction until plan execution rather than during planning?
If so it seems like a good idea to me
// specific language governing permissions and limitations | ||
// under the License. | ||
|
||
//! A simplified [`TableProvider`] for streaming partitioned datasets |
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.
Is the idea that this is the counterpart for MemTable
but with deferred execution?
/// Construct the `information_schema.tables` virtual table | ||
fn make_tables(&self) -> Arc<dyn TableProvider> { | ||
fn make_tables(&self, builder: &mut InformationSchemaTablesBuilder) { |
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.
maybe we can refactor them in a follow on PR
} | ||
} | ||
|
||
impl PartitionStream for InformationSchemaTables { |
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.
but more verbose than creating a MemTable
, right?
Benchmark runs are scheduled for baseline = 1c6b143 and contender = f8a3d58. f8a3d58 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Relates to #3777
Rationale for this change
Whilst working on #3777 I discovered somewhat to my surprise that the work of computing system tables is actually done eagerly in
SchemaProvider::table
. This is at best surprisingWhat changes are included in this PR?
Adds a
StreamingTable
that is akin toMemTable
but streaming, and then uses this to implement the system tables.Are these changes tested?
Are there any user-facing changes?