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

Upgrading libraries, mainly the JSqlParser, to support more queries out of the box. #41

Merged
merged 5 commits into from
Jan 26, 2024

Conversation

onukristo
Copy link
Contributor

@onukristo onukristo commented Jan 25, 2024

Context

Upgrading libraries, mainly the JSqlParser, to support more queries out of the box.

Checklist

@onukristo onukristo added the change:standard Not an emergency or impactful change label Jan 25, 2024
@onukristo onukristo requested a review from a team as a code owner January 25, 2024 09:38
import net.sf.jsqlparser.JSQLParserException;
import net.sf.jsqlparser.parser.CCJSqlParser;
import net.sf.jsqlparser.parser.StringProvider;
import net.sf.jsqlparser.statement.Statements;

public class SqlParser {

/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New version supports the queries we tried to modify.

@@ -120,18 +127,26 @@ protected ParsedQuery parseSql(String sql, TwContext context) {
return new ParsedQuery();
}

ParsedQuery result = new ParsedQuery();
long startTimeMs = System.currentTimeMillis();
var result = new ParsedQuery();
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I don't see how using var here makes it better. long vs var, ParsedQuery vs var. In this case using var hurts the readability more in my opinion.

}
}

for (Statement stmt : stmts) {
Copy link
Contributor

@tw-peeterkarolin tw-peeterkarolin Jan 26, 2024

Choose a reason for hiding this comment

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

Line 135:
for (var stmt : stmts) {
Line 141:
for (Statement stmt : stmts) {

var vs proper class name.

@onukristo onukristo merged commit e841df2 into master Jan 26, 2024
18 checks passed
@s0
Copy link

s0 commented Feb 14, 2024

/wise-bot run-action sync-codeowners

@wise-github-bot-app
Copy link

Your CODEOWNERS or the tw-rules.yaml file has changed. Syncing your GitHub teams with your CODEOWNERS file and extraWriters in tw-rules:

Collaborator Name Synced Comment
@tw-census 🟢 User added with role admin
application-engineering 🟢 Team was already up to date

You have more info in the docs

@wise-github-bot-app
Copy link

🟢

sync-codeowners action completed with successful result.

@mscott-tw mscott-tw deleted the sql_parser_upgrade branch February 15, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:standard Not an emergency or impactful change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants