github-actions[bot] commented on code in PR #65100:
URL: https://github.com/apache/doris/pull/65100#discussion_r3510452282
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java:
##########
@@ -769,16 +769,45 @@ public void setName(String name) {
}
public synchronized void addFunction(Function function, boolean
ifNotExists) throws UserException {
- function.checkWritable();
- if (FunctionUtil.addFunctionImpl(function, ifNotExists, false,
name2Function)) {
- Env.getCurrentEnv().getEditLog().logAddFunction(function);
- try {
+ addFunctions(ImmutableList.of(function), ifNotExists);
+ }
+
+ public synchronized void addTableFunction(Function function, boolean
ifNotExists) throws UserException {
+ // Doris table functions are registered as two functions: the normal
function and its outer variant.
+ Function outerFunction = function.clone();
+ FunctionName name = outerFunction.getFunctionName();
+ name.setFn(name.getFunction() + "_outer");
+ addFunctions(ImmutableList.of(function, outerFunction), ifNotExists);
+ }
+
+ private void addFunctions(List<Function> functions, boolean ifNotExists)
throws UserException {
+ List<Function> addedFunctions = Lists.newArrayList();
+ try {
+ for (Function function : functions) {
+ function.checkWritable();
+ if (FunctionUtil.addFunctionImpl(function, ifNotExists, false,
name2Function)) {
Review Comment:
This still is not atomic when the statement uses `IF NOT EXISTS`. For
example, if a user already has an unrelated `f_outer(INT)` and no `f(INT)`,
`CREATE TABLES FUNCTION IF NOT EXISTS f(INT) ...` adds `f` at this line, then
the generated `f_outer` hits the existing signature and `addFunctionImpl(...,
true, ...)` returns `false` instead of failing. The batch then translates and
logs only `f`, leaving the table function without its real companion; worse, a
later `DROP FUNCTION f(INT)` will see `f` is a UDTF and unconditionally drop
`f_outer(INT)`, deleting the pre-existing unrelated function. Please make the
table-function pair precheck/rollback as a unit here: `IF NOT EXISTS` should
skip only when the whole pair already exists as the expected table-function
pair, and a companion-name collision while creating the base should abort
without registering either half.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java:
##########
@@ -769,16 +769,45 @@ public void setName(String name) {
}
public synchronized void addFunction(Function function, boolean
ifNotExists) throws UserException {
- function.checkWritable();
- if (FunctionUtil.addFunctionImpl(function, ifNotExists, false,
name2Function)) {
- Env.getCurrentEnv().getEditLog().logAddFunction(function);
- try {
+ addFunctions(ImmutableList.of(function), ifNotExists);
+ }
+
+ public synchronized void addTableFunction(Function function, boolean
ifNotExists) throws UserException {
+ // Doris table functions are registered as two functions: the normal
function and its outer variant.
+ Function outerFunction = function.clone();
+ FunctionName name = outerFunction.getFunctionName();
+ name.setFn(name.getFunction() + "_outer");
+ addFunctions(ImmutableList.of(function, outerFunction), ifNotExists);
+ }
+
+ private void addFunctions(List<Function> functions, boolean ifNotExists)
throws UserException {
+ List<Function> addedFunctions = Lists.newArrayList();
+ try {
+ for (Function function : functions) {
+ function.checkWritable();
+ if (FunctionUtil.addFunctionImpl(function, ifNotExists, false,
name2Function)) {
+ addedFunctions.add(function);
+ }
+ }
+ for (Function function : addedFunctions) {
FunctionUtil.translateToNereidsThrows(this.getFullName(),
function);
- } catch (Exception e) {
- name2Function.remove(function.getFunctionName().getFunction());
- throw e;
}
+ } catch (Exception e) {
+ for (Function function : addedFunctions) {
+ FunctionUtil.dropFromNereids(this.getFullName(),
getFunctionSearchDesc(function));
+ }
+ for (int i = addedFunctions.size() - 1; i >= 0; i--) {
+ FunctionUtil.removeFunctionImpl(addedFunctions.get(i),
name2Function);
+ }
+ throw e;
}
+ for (Function function : addedFunctions) {
+ Env.getCurrentEnv().getEditLog().logAddFunction(function);
Review Comment:
The successful table-function path still journals the pair as two
independent records. After the batch has validated and translated both
functions, this loop writes one `OP_ADD_FUNCTION` for the base and then one for
`_outer`; `EditLog` replay restores each record independently via
`replayCreateFunction()`/`Database.replayAddFunction()`. If the FE crashes
after the base record is durable but before the `_outer` record is written,
restart exposes only the base UDTF in both the catalog and Nereids registry.
Please persist the table-function pair as one atomic journal payload, or add
replay-side repair/rollback so a restart cannot leave only one half of a
successfully created table function.
##########
fe/fe-core/src/test/java/org/apache/doris/catalog/CreateFunctionTest.java:
##########
@@ -170,6 +176,151 @@ public void testCreatePythonFunctionRejectsObjectTypes()
throws Exception {
"ARRAY unsupported sub-type: bitmap");
}
+ @Test
+ public void testCreateFunctionRollbackOnlyFailedOverload() throws
Exception {
+ ConnectContext ctx = UtFrameUtils.createDefaultCtx();
+ createDatabase(ctx, "create database rollback_function_db;");
+ Database db =
Env.getCurrentInternalCatalog().getDbNullable("rollback_function_db");
+ Assert.assertNotNull(db);
+
+ EditLog editLog = Env.getCurrentEnv().getEditLog();
+ EditLog spyEditLog = Mockito.spy(editLog);
+
Mockito.doNothing().when(spyEditLog).logAddFunction(Mockito.any(Function.class));
+ Env.getCurrentEnv().setEditLog(spyEditLog);
+ try (MockedStatic<FunctionUtil> mockedFunctionUtil =
Mockito.mockStatic(FunctionUtil.class,
+ Mockito.CALLS_REAL_METHODS)) {
+ Function existingFunction = createJavaUdf("rollback_function_db",
"rollback_fn", Type.INT);
+ db.addFunction(existingFunction, false);
+ Assert.assertSame(existingFunction,
db.getFunction(searchDesc(existingFunction)));
+
+ Mockito.clearInvocations(spyEditLog);
+ Function failedFunction = createJavaUdf("rollback_function_db",
"rollback_fn", Type.BIGINT);
+ mockedFunctionUtil.when(() ->
FunctionUtil.translateToNereidsThrows(
+ "rollback_function_db", failedFunction)).thenThrow(new
RuntimeException("translate failed"));
+
+ RuntimeException exception =
Assert.assertThrows(RuntimeException.class,
+ () -> db.addFunction(failedFunction, false));
+ Assert.assertEquals("translate failed", exception.getMessage());
+ Mockito.verify(spyEditLog,
Mockito.never()).logAddFunction(Mockito.any(Function.class));
+ Assert.assertSame(existingFunction,
db.getFunction(searchDesc(existingFunction)));
+ Assert.assertThrows(AnalysisException.class, () ->
db.getFunction(searchDesc(failedFunction)));
+ } finally {
+ Env.getCurrentEnv().setEditLog(editLog);
+ }
+ }
+
+ @Test
+ public void testCreateTableFunctionRollbackWhenOuterFunctionFails() throws
Exception {
+ ConnectContext ctx = UtFrameUtils.createDefaultCtx();
+ createDatabase(ctx, "create database rollback_table_function_db;");
+ Database db =
Env.getCurrentInternalCatalog().getDbNullable("rollback_table_function_db");
+ Assert.assertNotNull(db);
+
+ EditLog editLog = Env.getCurrentEnv().getEditLog();
+ EditLog spyEditLog = Mockito.spy(editLog);
+
Mockito.doNothing().when(spyEditLog).logAddFunction(Mockito.any(Function.class));
+ Env.getCurrentEnv().setEditLog(spyEditLog);
+ try (MockedStatic<FunctionUtil> mockedFunctionUtil =
Mockito.mockStatic(FunctionUtil.class,
+ Mockito.CALLS_REAL_METHODS)) {
+ Function tableFunction =
createJavaUdtf("rollback_table_function_db", "rollback_table_fn", Type.INT);
+ mockedFunctionUtil.when(() ->
FunctionUtil.translateToNereidsThrows(
+ Mockito.eq("rollback_table_function_db"),
Mockito.any(Function.class)))
+ .thenAnswer(invocation -> {
+ Function function = invocation.getArgument(1);
+ if
("rollback_table_fn_outer".equals(function.functionName())) {
+ throw new RuntimeException("outer translate
failed");
+ }
+ return true;
+ });
+
+ RuntimeException exception =
Assert.assertThrows(RuntimeException.class,
+ () -> db.addTableFunction(tableFunction, false));
+ Assert.assertEquals("outer translate failed",
exception.getMessage());
+ Mockito.verify(spyEditLog,
Mockito.never()).logAddFunction(Mockito.any(Function.class));
+ Assert.assertThrows(AnalysisException.class, () ->
db.getFunction(searchDesc(tableFunction)));
+ Assert.assertThrows(AnalysisException.class,
+ () ->
db.getFunction(searchDesc("rollback_table_function_db",
"rollback_table_fn_outer",
+ Type.INT)));
+ mockedFunctionUtil.verify(() ->
FunctionUtil.dropFromNereids(Mockito.eq("rollback_table_function_db"),
+ Mockito.argThat(function ->
"rollback_table_fn".equals(function.getName().getFunction()))));
+ mockedFunctionUtil.verify(() ->
FunctionUtil.dropFromNereids(Mockito.eq("rollback_table_function_db"),
+ Mockito.argThat(function ->
"rollback_table_fn_outer".equals(function.getName().getFunction()))));
+ } finally {
+ Env.getCurrentEnv().setEditLog(editLog);
+ }
+ }
Review Comment:
This assertion will not match the `UserException` thrown by the conflict
path. `FunctionUtil.addFunctionImpl()` throws `new UserException("function
already exists")`, but `UserException.getMessage()` formats it as `errCode = 2,
detailMessage = function already exists`; the bare text is returned by
`getDetailMessage()`. As written, this new test fails on the assertion instead
of validating rollback. Please assert `exception.getDetailMessage()` here, or
compare `getMessage()` against the full formatted string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
