Skip to content

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

shuyouZZ
Copy link
Contributor

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?

mvn test -Dtest.groups= -Dtest=org.apache.hadoop.hive.metastore.TestObjectStore#testDatabaseOps -pl :hive-standalone-metastore-server


@Override
protected List<String> getJdoResult(GetHelper<List<String>> ctx) throws MetaException {
return getGetDatabasesViaJdo(catName, pattern);
Copy link
Contributor

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

Copy link
Contributor Author

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.

@okumin
Copy link
Contributor

okumin commented Jun 16, 2025

CI failed probably because of this change.

@shuyouZZ
Copy link
Contributor Author

CI failed probably because of this change.

I can not open the continuous-integration page, it shows Not Found.
The previous execution of CI was successful.

@okumin
Copy link
Contributor

okumin commented Jun 16, 2025

@shuyouZZ You can sign in the Jenkins with a GitHub account

@shuyouZZ
Copy link
Contributor Author

@shuyouZZ You can sign in the Jenkins with a GitHub account

thanks, let me try

Copy link

@deniskuzZ
Copy link
Member

is that PR abandoned?

@shuyouZZ
Copy link
Contributor Author

shuyouZZ commented Aug 8, 2025

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.

@okumin
Copy link
Contributor

okumin commented Aug 9, 2025

@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
https://github.com/apache/hive/actions/runs/15722082143/job/44304757814?pr=5852

@shuyouZZ
Copy link
Contributor Author

shuyouZZ commented Aug 9, 2025

@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 https://github.com/apache/hive/actions/runs/15722082143/job/44304757814?pr=5852

Ok. Let me try force rebase.

Copy link

sonarqubecloud bot commented Aug 9, 2025


private List<String> getDatabasesInternal(String catName, String pattern)
throws MetaException, NoSuchObjectException {
return new GetHelper<List<String>>(catName, null, pattern, true, true) {
Copy link
Contributor

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('*', '%'));
}
Copy link
Contributor

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.

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");

Copy link
Contributor

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();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants