-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[FLINK-27376][sql] Support current_database function #19218
Conversation
2a63613
to
9590059
Compare
9590059
to
455edd9
Compare
9fdaa74
to
2ddd919
Compare
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.
Thanks for your contribution. I am not sure whether user with hive module in default dialect still get the same problem.
} catch (SemanticException e) { | ||
throw new FlinkHiveException(e); | ||
} | ||
return convertToLiteral(hiveShim.toHiveTimestamp(currentTS)); |
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.
It seems we can use
if (convertedOp instanceof SqlCastFunction) {
} else if (convertedOp instanceof FlinkSqlTimestampFunction) {
} else if (convertedOp.getName().equals("current_database")) {
} else {
return builder.makeCall(convertedOp, visitList(operands, update));
}
throw new FlinkHiveException(e); | ||
} | ||
return convertToLiteral(hiveShim.toHiveTimestamp(currentTS)); | ||
} else if (convertedOp.getName().equals("current_database")) { |
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 think it's very hacked to fix this problem in this way. Currently, we determine which function is used by comparing the method name, parameters, and return type. But here we only consider the function name.
I think we should introduce a class similar to SqlFunctionCast
here and use instanceOf
to determine.
tableEnv.executeSql("select current_database()").collect()); | ||
assertThat(result.toString()).isEqualTo("[+I[db1]]"); | ||
// switch to default database for following test use default database | ||
tableEnv.executeSql("use default"); |
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.
It's better if we can use
@After
public void cleanup() {
tableEnv.executeSql("use default");
}
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 think we don't need to add a cleanup
method for currently only one test needs it. It would be better to limit use default
in this single test.
2ddd919
to
72c7f36
Compare
@fsk119 Thanks for review. Now, I add function |
22d297c
to
163bc2d
Compare
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.
Thanks for update. It's better we can keep the origin tests in the hive.
// -------------------------------------------------------------------------------------------- | ||
// Catalog functions | ||
// -------------------------------------------------------------------------------------------- | ||
public static final BuiltInFunctionDefinition CURRENT_DATABASE = |
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.
Leave a blank line before the CURRENT_DATABASE
. We should align with others.
@@ -233,6 +233,10 @@ object StringCallGen { | |||
isCharacterString(operands(2).resultType) => | |||
methodGen(BuiltInMethods.CONVERT_TZ) | |||
|
|||
case CURRENT_DATABASE => | |||
val timestamp = ctx.addReusableQueryLevelCurrentDatabase() |
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.
timestamp -> database
@@ -52,6 +52,13 @@ public final class InternalConfigOptions { | |||
+ " some temporal functions like LOCAL_TIMESTAMP in batch job to make sure these" | |||
+ " temporal functions has query-start semantics."); | |||
|
|||
public static final ConfigOption<String> TABLE_QUERY_CURRENT_DATABASE = | |||
key("__table.query-start.current_database__") |
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.
Other options use the "-" rather than "_". How about renaming to __table.query-start.current-database__
?
.stringType() | ||
.noDefaultValue() | ||
.withDescription( | ||
"The config used to save the current database at query start."); |
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.
Also, document who will use the value?
f834d97
to
b93e796
Compare
@fsk119 Thanks for reviewing. I have updated it. |
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.
LGTM.
But the CI test fails. Could you take a look?
520d407
to
e6f8f44
Compare
e6f8f44
to
3c137ae
Compare
@flinkbot run azure |
@fsk119 The ci is passed now. Could you please help merge when you're free? |
What is the purpose of the change
To support
current_database()
function in FlinkBrief change log
CURRENT_DATABASE
definition in BuiltInFunctionDefinitions.java and FlinkSqlOperatorTable.javacurrentDatabase
to callCURRENT_DATABASE
in Expressions.java to make current_database can be used in TableAPI.PlannerBase
. And read the configuration value put to get the current in codegen phase forcurrent_database()
function.current_database
in HiveMoudle, so that it'll always use Flink's built-incurrent_database
function.Verifying this change
Added test in testCurrentDatabase, testCurrentDatabase, testCurrentDatabase, testCurrentDatabase for batch/stream, sql/table.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation