-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-28994: Implement direct sql for getAllDatabases and getDatabases in RawStore #5852
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
base: master
Are you sure you want to change the base?
Conversation
|
||
@Override | ||
protected List<String> getJdoResult(GetHelper<List<String>> ctx) throws MetaException { | ||
return getGetDatabasesViaJdo(catName, pattern); |
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.
getGetDatabasesViaJdo has redundant get in the method name which can be removed
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.
getGetDatabasesViaJdo has redundant get in the method name which can be removed
thanks, have fix it.
CI failed probably because of this change. |
I can not open the continuous-integration page, it shows Not Found. |
@shuyouZZ You can sign in the Jenkins with a GitHub account |
thanks, let me try |
|
is that PR abandoned? |
Sorry for taking so long to reply. I still hope to continue working on this PR, but I'm not sure why this build failed. |
@shuyouZZ Although this might not be the direct reason, you have to rebase the topic branch. The CI says it was tested with JDK 17, but the upstream uses JDK 21 |
Ok. Let me try force rebase. |
|
|
||
private List<String> getDatabasesInternal(String catName, String pattern) | ||
throws MetaException, NoSuchObjectException { | ||
return new GetHelper<List<String>>(catName, null, pattern, true, true) { |
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.
I'd say two potential updates on this line.
First, I suspect this should be new GetHelper<>(catname, null, null, true, true)
because GetHelper can unlikely handle a pattern. @dengzhhu653 We would like your advice on this hypothesis.
Second, if GetHelper
accepts a pattern, this should be GetHelper<>(catName, pattern, null, true, true)
because the given pattern is not that of tables but that of databases.
if (i > 0) queryText.append(" OR "); | ||
queryText.append("\"NAME\" LIKE ?"); | ||
params.add(validPatterns.get(i).replace('*', '%')); | ||
} |
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.
Does this loop retain the original semantics? I can see the original implementation assumes pattern
is a regular expression, which is not identical to that of SQL's LIKE
.
Lines 2027 to 2051 in 411d3a4
private StringBuilder appendCondition(StringBuilder builder, | |
String fieldName, String[] elements, boolean pattern, List<String> parameters) { | |
if (builder.length() > 0) { | |
builder.append(" && "); | |
} | |
builder.append(" ("); | |
int length = builder.length(); | |
for (String element : elements) { | |
if (pattern) { | |
element = "(?i)" + element.replaceAll("\\*", ".*"); | |
} | |
parameters.add(element); | |
if (builder.length() > length) { | |
builder.append(" || "); | |
} | |
builder.append(fieldName); | |
if (pattern) { | |
builder.append(".matches(").append(JDO_PARAM).append(parameters.size()).append(")"); | |
} else { | |
builder.append(" == ").append(JDO_PARAM).append(parameters.size()); | |
} | |
} | |
builder.append(" )"); | |
return builder; | |
} |
} | ||
|
||
queryText.append(" ORDER BY \"NAME\" ASC"); | ||
|
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.
We might want to instantiate and reuse the query text.
String properVariableName = queryText.toString();
What changes were proposed in this pull request?
Implement direct sql for getAllDatabases and getDatabases in RawStore.
Why are the changes needed?
Improve the performance of getAllDatabases and getDatabases.
Does this PR introduce any user-facing change?
No
How was this patch tested?