-
Notifications
You must be signed in to change notification settings - Fork 909
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
Graphman copy indexing improvements #5425
base: master
Are you sure you want to change the base?
Conversation
4450622
to
a656ea2
Compare
6287b01
to
1c4f291
Compare
cbd2732
to
2204c6e
Compare
844d9e2
to
35dbb91
Compare
4881b62
to
668b7c4
Compare
668b7c4
to
042641c
Compare
@@ -155,31 +155,31 @@ fn test_manual_index_creation_ddl() { | |||
#[test] | |||
fn generate_ddl() { | |||
let layout = test_layout(THING_GQL); | |||
let sql = layout.as_ddl().expect("Failed to generate DDL"); | |||
let sql = layout.as_ddl(false).expect("Failed to generate DDL"); |
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.
At least for one of these, there should also be a test that passes true
to demonstrate which indexes are created in that case.
@@ -180,6 +180,7 @@ impl DeploymentStore { | |||
graft_base: Option<Arc<Layout>>, | |||
replace: bool, | |||
on_sync: OnSync, | |||
is_copy_op: bool, |
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.
It might be clearer to use a separate enum here rather than passing a boolean flag, maybe something like enum IndexMode { Immediate, Delay }
. That enum could then also encapsulate some of the logic around when to create an index and when not, like mode.should_create(column, index_method) -> bool
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.
Meanwhile I removed this flag altogether and replaced if with Option<IndexList>
. Do you think that way it's readable enough or I should still go for an explicit enum?
.columns | ||
.iter() | ||
.map(|i| i.name.to_string()) | ||
.collect::<Vec<String>>(); |
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 very minor here, and can stay like this, since this code isn't run very often, but in general it is better to avoid allocations like that; if this code was called more often, the helpers above should take a Table
and iterate over its columns without allocating
.collect::<Vec<String>>(); | ||
|
||
match self { | ||
CreateIndex::Unknown { defn: _ } => (), |
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 it correct that we return true
for an index that we couldn't parse? It would be clearer if this arm had a return
in it.
36281a8
to
a8d7284
Compare
Addresses the #5140 issue. The tests are still pending.