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.

Sign up for GitHub

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

Jump to bottom

Fix bug in locating fields when a JOIN clause is present #102

Merged
merged 9 commits into from
Jul 31, 2023
Merged

Fix bug in locating fields when a JOIN clause is present #102

merged 9 commits into from
Jul 31, 2023

Conversation

mamcx
Copy link
Contributor

@mamcx mamcx commented Jul 26, 2023 •
edited
Loading

Description of Changes

When a JOIN clause is presented before was constructed a new table name like TableA | TableB, this cause locating fields for TableA or TableB fails.

Also making more robust use of the hidden field OP_TYPE_FIELD_NAME, not assuming the location of it in the header.

Adding tracing for the SQL compilation step to see which SQL are running and more details in panics for subscription related to this PR.

API

  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • This is a breaking change to the SDK API

If the API is breaking, please state below what will break

Centril
Centril reviewed Jul 26, 2023
View reviewed changes
crates/lib/src/relation.rs Outdated Show resolved Hide resolved
crates/lib/src/relation.rs Outdated Show resolved Hide resolved
crates/lib/src/relation.rs Outdated Show resolved Hide resolved
crates/lib/src/relation.rs Outdated Show resolved Hide resolved
crates/core/src/subscription/subscription.rs Outdated Show resolved Hide resolved
crates/core/src/subscription/subscription.rs Outdated Show resolved Hide resolved
crates/core/src/subscription/subscription.rs Outdated Show resolved Hide resolved
crates/core/src/sql/ast.rs Outdated Show resolved Hide resolved
crates/core/src/subscription/query.rs Outdated Show resolved Hide resolved
Centril
Centril reviewed Jul 27, 2023
View reviewed changes
crates/lib/src/relation.rs Show resolved Hide resolved
crates/core/src/db/datastore/traits.rs
@@ -169,6 +169,9 @@ pub struct TableSchema {
}

impl TableSchema {
/// Check if the `name` of the [FieldName] exist on this [TableSchema]
///
/// Warning: It ignores the `table_name`
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm; the relevance of this comment isn't entirely clear to me. Could you elaborate on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will rework the field resolution because is broken now but is unrelated to the fix for this PR.

crates/core/src/db/datastore/traits.rs
self.columns.iter().find(|x| x.col_name == name)
/// Check if the `col_name` exist on this [TableSchema]
///
/// Warning: It ignores the `table_name`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same re. relevance.

mamcx and others added 9 commits July 27, 2023 14:50
@mamcx
Fix bug in locating fields when a JOIN clause is present
137970c
@mamcx @Centril
Update crates/lib/src/relation.rs
d0314a0 
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>
@mamcx @Centril
Update crates/lib/src/relation.rs
8899992 
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>
@mamcx @Centril
Update crates/core/src/subscription/subscription.rs
ddefd9c 
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>
@mamcx @Centril
Update crates/core/src/subscription/subscription.rs
2785c06 
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>
@mamcx @Centril
Update crates/core/src/subscription/subscription.rs
df80872 
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>
@mamcx @Centril
Update crates/core/src/subscription/query.rs
d1b4a4f 
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>
@mamcx
Small doc nits
015e475
@mamcx
Fix test
6f260f6
cloutiertyler
cloutiertyler approved these changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

LGTM + tested by Alessandro

@cloutiertyler cloutiertyler merged commit 333bbc3 into master Jul 31, 2023
4 checks passed
cloutiertyler pushed a commit that referenced this pull request Aug 1, 2023
@mamcx @Centril
Fix bug in locating fields when a JOIN clause is present (#102)
d3c2b42 
* Fix bug in locating fields when a JOIN clause is present

* Update crates/lib/src/relation.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/lib/src/relation.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/subscription.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/subscription.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/subscription.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/query.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Small doc nits

* Fix test

---------

Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
cloutiertyler pushed a commit that referenced this pull request Aug 1, 2023
@mamcx @Centril @cloutiertyler
Fix bug in locating fields when a JOIN clause is present (#102)
6bd3237 
* Fix bug in locating fields when a JOIN clause is present

* Update crates/lib/src/relation.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/lib/src/relation.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/subscription.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/subscription.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/subscription.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/query.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Small doc nits

* Fix test

---------

Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
cloutiertyler pushed a commit that referenced this pull request Aug 1, 2023
@mamcx @Centril @cloutiertyler
Fix bug in locating fields when a JOIN clause is present (#102)
20ecb98 
* Fix bug in locating fields when a JOIN clause is present

* Update crates/lib/src/relation.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/lib/src/relation.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/subscription.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/subscription.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/subscription.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Update crates/core/src/subscription/query.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

* Small doc nits

* Fix test

---------

Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
@cloutiertyler cloutiertyler deleted the mamcx/bug_op_type branch August 1, 2023 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril left review comments

@cloutiertyler cloutiertyler cloutiertyler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mamcx @Centril @cloutiertyler