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.

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

Extend Acra with latest changes #547

Merged
merged 7 commits into from
Jun 6, 2022
Merged

Conversation

Zhaars
Copy link
Contributor

@Zhaars Zhaars commented May 31, 2022

Updated Acra with a couple of latest fixes.

  • Updated encryptorConfig to support TA with searchable encryption;
  • Extend sqlparser to support NULLS FIRST/LAST statements;
  • Improvements of the mapColumnsToAliases function to support some JOINS queries and queries wish subqueries;

Added tests to cover corresponding functionality;

Checklist

Zhaars added 6 commits May 5, 2022 19:19
Extend sqlparser to support set key TO value gramar
Added sqlparser unit tests
Fixed after review
Remove redundant things
Last changes improvements
…atest_changes

# Conflicts:
#	sqlparser/sql.go
@Zhaars Zhaars requested a review from Lagovas May 31, 2022 13:43
}

if _, ok := subSelect.SelectExprs[0].(*sqlparser.StarExpr); ok {
return nil, errUnsupportedExpression
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should leave TODO that we plan to support it with passed encryptor_config at the future?

Copy link
Contributor Author

@Zhaars Zhaars Jun 2, 2022

Choose a reason for hiding this comment

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

Hmm, but all this block is related to handling such kinds of queries.

SELECT a, (SELECT b FROM table2) FROM table1;

So here we have some checks that select expression should be only one in any case and we don't need to use encryptor_config here. Am I wrong?

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, I missed that there is subSelect, not starSelect that requires encryptor_config

@@ -48,19 +48,23 @@ func parseJoinTablesInfo(joinExp *sqlparser.JoinTableExpr, tables *[]string, ali
if ok {
// here we reach the last leaf in the JoinTableExpr recursive tree, processing SHOULD be stopped in this block.
// and we should process remaining RightExpr and LeftExpr leafs more before exit.
name, alias, ok := getRightJoinTableInfo(joinExp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you update comment for this function with updated internal logic? what and how it actually parse information about tables

if !ok {
t.Fatal("Test query should be Select expression")
}
`select t1.number AS t1_number, t2.number AS t2_number from (select * from tablex) AS t JOIN (table1 AS t1 JOIN table2 AS t2 ON t1.id = t2.exam_type_id) ON t.version_id =
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add comments what actually we check here and what difference between cases?
additionally lets verify reverse ordered columns in SELECT statement? If we join t1 with t2 and t3 then lets check SELECT t3.*, t1.*, t2.* and check that they should not be ordered in same way

@Zhaars Zhaars requested a review from Lagovas June 2, 2022 09:46
func getRightJoinTableInfo(joinExp *sqlparser.JoinTableExpr) (string, string, bool) {
// in case of more complex JOINs constructions like `JOIN (table1 AS t1 JOIN table2 AS t2 ON ... JOIN table3 ...) ON ...`
// represented by sqlparser.ParenTableExpr it runs parseJoinTablesInfo itself recursively to collect tableName and its alias info inside this block
func getRightJoinTableInfo(joinExp *sqlparser.JoinTableExpr, tables *[]string, aliases map[string]string) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you explain here why we rely only on RightExpr and doesn't touch LeftExpr?

@Zhaars Zhaars merged commit f40e5c0 into master Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants