Use btree to search fields in DFSchema #7870
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oleggator
force-pushed
the
dfschema-optimization-main
branch
from
1966c0c to
95fe304
Compare
|
Is there a reason to use a b-tree ( |
alamb
reviewed
Oct 23, 2023
| @@ -102,8 +217,12 @@ impl DFSchema { | |||
| )); | |||
| } | |||
| } | |||
|
|
|||
| let fields_index = build_index(&fields); | |||
There was a problem hiding this comment.
If the index is built for all DFSchema that are created, I wonder if that will be too much overhead. Maybe we could consider creating it on first use 🤔 or finding some way to canonicalize / cache the map
There was a problem hiding this comment.
this use case might indeed be a good call of interior mutability, i.e. use an RWLock and init the lookup table on the first use
There was a problem hiding this comment.
Thinking about this more: instead of RwLock, this can be solved even more elegantly w/ OnceLock::get_or_init (this is usually used for static variables, but you can totally use that for struct members as well).
|
I plan to review this and related PRs tomorrow morning |
|
Related comment: #7698 (comment) |
Using b-tree we can query all fields matching to a "prefix" in one O(logn) hop ( |
Weijun-H
reviewed
Oct 27, 2023
Comment on lines
+63
to
+67
| match self.field().cmp(other.field()) { | ||
| Ordering::Less => return Ordering::Less, | ||
| Ordering::Greater => return Ordering::Greater, | ||
| Ordering::Equal => {} | ||
| } |
There was a problem hiding this comment.
Suggested change
| match self.field().cmp(other.field()) { | |
| Ordering::Less => return Ordering::Less, | |
| Ordering::Greater => return Ordering::Greater, | |
| Ordering::Equal => {} | |
| } | |
| let field_cmp = self.field().cmp(other.field()); | |
| if field_cmp != Ordering::Equal { | |
| return field_cmp; | |
| } |
Comment on lines
+70
to
+74
| (Some(lhs), Some(rhs)) => match lhs.cmp(rhs) { | ||
| Ordering::Less => return Ordering::Less, | ||
| Ordering::Greater => return Ordering::Greater, | ||
| Ordering::Equal => {} | ||
| }, |
There was a problem hiding this comment.
Suggested change
| (Some(lhs), Some(rhs)) => match lhs.cmp(rhs) { | |
| Ordering::Less => return Ordering::Less, | |
| Ordering::Greater => return Ordering::Greater, | |
| Ordering::Equal => {} | |
| }, | |
| (Some(lhs), Some(rhs)) => { | |
| let cmp = lhs.cmp(rhs); | |
| if cmp != Ordering::Equal { | |
| return cmp; | |
| } | |
| } |
Comment on lines
+81
to
+85
| (Some(lhs), Some(rhs)) => match lhs.cmp(rhs) { | ||
| Ordering::Less => return Ordering::Less, | ||
| Ordering::Greater => return Ordering::Greater, | ||
| Ordering::Equal => {} | ||
| }, |
There was a problem hiding this comment.
Suggested change
| (Some(lhs), Some(rhs)) => match lhs.cmp(rhs) { | |
| Ordering::Less => return Ordering::Less, | |
| Ordering::Greater => return Ordering::Greater, | |
| Ordering::Equal => {} | |
| }, | |
| (Some(lhs), Some(rhs)) => { | |
| let cmp = lhs.cmp(rhs); | |
| if cmp != Ordering::Equal { | |
| return cmp; | |
| } | |
| } |
Comment on lines
+92
to
+96
| (Some(lhs), Some(rhs)) => match lhs.cmp(rhs) { | ||
| Ordering::Less => return Ordering::Less, | ||
| Ordering::Greater => return Ordering::Greater, | ||
| Ordering::Equal => {} | ||
| }, |
There was a problem hiding this comment.
Suggested change
| (Some(lhs), Some(rhs)) => match lhs.cmp(rhs) { | |
| Ordering::Less => return Ordering::Less, | |
| Ordering::Greater => return Ordering::Greater, | |
| Ordering::Equal => {} | |
| }, | |
| (Some(lhs), Some(rhs)) => { | |
| let cmp = lhs.cmp(rhs); | |
| if cmp != Ordering::Equal { | |
| return cmp; | |
| } | |
| } |
| /// DFSchema wraps an Arrow schema and adds relation names | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct DFSchema { | ||
| /// Fields | ||
| fields: Vec<DFField>, | ||
| /// Fields index | ||
| fields_index: BTreeMap<OwnedFieldReference, Vec<usize>>, |
There was a problem hiding this comment.
I think we use BTree here because it ensures the order of the index 🤔.
There was a problem hiding this comment.
we do you care about the index order? You either iterate over the fields in order (use self.fields.iter()) or you lookup a field by name (use self.field_index.get(...)). The index is orderd by field name. So this argument would only be valid if we OFTEN iterate over the fields in name order, which I don't think we do.
Is that such a common operation that it is worth to keep an expensive index on every single schema in the query graph? I think the planner that resolves these names can easily order the fields and build this index locally. |
|
Made a benchmark. Baseline - Data Fusion 32 (a0c5aff)This PR |
Could you please add summary? It seems that btree provides an advantage with 100+ cols |
|
Thank you -- I plan to review this more carefully tomorrow |
@alamb I think it's a good idea to introduce user defined cacheprovider for both DFSchema and arrow Schema. It will allow to take benefits from btree and avoid building it when is not necessary. |
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of #7698.
Rationale for this change
Current DFSchema implementation uses vector to operate with fields. It makes search of a column by name algorithmically complex.
What changes are included in this PR?
Use BTreeMap to index field qualifiers.
Are these changes tested?
Are there any user-facing changes?
No