github-actions[bot] commented on code in PR #65100:
URL: https://github.com/apache/doris/pull/65100#discussion_r3510664427
##########
fe/fe-core/src/test/java/org/apache/doris/catalog/CreateFunctionTest.java:
##########
@@ -170,6 +176,229 @@ 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));
+
Mockito.doNothing().when(spyEditLog).logAddFunctions(Mockito.anyList());
+ 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));
+ Mockito.verify(spyEditLog,
Mockito.never()).logAddFunctions(Mockito.anyList());
+ 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);
+ }
+ }
+
+ @Test
+ public void testCreateTableFunctionRollbackWhenOuterFunctionConflicts()
throws Exception {
+ ConnectContext ctx = UtFrameUtils.createDefaultCtx();
+ createDatabase(ctx, "create database
rollback_table_function_conflict_db;");
+ Database db =
Env.getCurrentInternalCatalog().getDbNullable("rollback_table_function_conflict_db");
+ Assert.assertNotNull(db);
+
+ EditLog editLog = Env.getCurrentEnv().getEditLog();
+ EditLog spyEditLog = Mockito.spy(editLog);
+
Mockito.doNothing().when(spyEditLog).logAddFunction(Mockito.any(Function.class));
+
Mockito.doNothing().when(spyEditLog).logAddFunctions(Mockito.anyList());
+ Env.getCurrentEnv().setEditLog(spyEditLog);
+ try (MockedStatic<FunctionUtil> mockedFunctionUtil =
Mockito.mockStatic(FunctionUtil.class,
+ Mockito.CALLS_REAL_METHODS)) {
+ mockedFunctionUtil.when(() ->
FunctionUtil.translateToNereidsThrows(
+ Mockito.eq("rollback_table_function_conflict_db"),
Mockito.any(Function.class)))
+ .thenReturn(true);
+ Function existingOuterFunction = createJavaUdtf(
+ "rollback_table_function_conflict_db",
"rollback_table_conflict_fn_outer", Type.INT);
+ db.addFunction(existingOuterFunction, false);
+ Assert.assertSame(existingOuterFunction,
db.getFunction(searchDesc(existingOuterFunction)));
+
+ Mockito.clearInvocations(spyEditLog);
+ Function tableFunction = createJavaUdtf(
+ "rollback_table_function_conflict_db",
"rollback_table_conflict_fn", Type.INT);
+ UserException exception = Assert.assertThrows(UserException.class,
+ () -> db.addTableFunction(tableFunction, true));
+ Assert.assertEquals("function already exists",
exception.getDetailMessage());
+ Mockito.verify(spyEditLog,
Mockito.never()).logAddFunction(Mockito.any(Function.class));
+ Mockito.verify(spyEditLog,
Mockito.never()).logAddFunctions(Mockito.anyList());
+ Assert.assertThrows(AnalysisException.class, () ->
db.getFunction(searchDesc(tableFunction)));
+ Assert.assertSame(existingOuterFunction,
db.getFunction(searchDesc(existingOuterFunction)));
+ } finally {
+ Env.getCurrentEnv().setEditLog(editLog);
+ }
+ }
+
+ @Test
+ public void testCreateTableFunctionIfNotExistsSkipsExistingPair() throws
Exception {
+ ConnectContext ctx = UtFrameUtils.createDefaultCtx();
+ createDatabase(ctx, "create database
existing_table_function_pair_db;");
+ Database db =
Env.getCurrentInternalCatalog().getDbNullable("existing_table_function_pair_db");
+ Assert.assertNotNull(db);
+
+ EditLog editLog = Env.getCurrentEnv().getEditLog();
+ EditLog spyEditLog = Mockito.spy(editLog);
+
Mockito.doNothing().when(spyEditLog).logAddFunction(Mockito.any(Function.class));
+
Mockito.doNothing().when(spyEditLog).logAddFunctions(Mockito.anyList());
+ Env.getCurrentEnv().setEditLog(spyEditLog);
+ try (MockedStatic<FunctionUtil> mockedFunctionUtil =
Mockito.mockStatic(FunctionUtil.class,
+ Mockito.CALLS_REAL_METHODS)) {
+ mockedFunctionUtil.when(() ->
FunctionUtil.translateToNereidsThrows(
+ Mockito.eq("existing_table_function_pair_db"),
Mockito.any(Function.class)))
+ .thenReturn(true);
+ Function existingFunction = createJavaUdtf(
+ "existing_table_function_pair_db",
"existing_table_pair_fn", Type.INT);
+ Function existingOuterFunction = createJavaUdtf(
+ "existing_table_function_pair_db",
"existing_table_pair_fn_outer", Type.INT);
+ db.addFunction(existingFunction, false);
+ db.addFunction(existingOuterFunction, false);
+ Assert.assertSame(existingFunction,
db.getFunction(searchDesc(existingFunction)));
+ Assert.assertSame(existingOuterFunction,
db.getFunction(searchDesc(existingOuterFunction)));
+
+ Mockito.clearInvocations(spyEditLog);
+ Function tableFunction = createJavaUdtf(
+ "existing_table_function_pair_db",
"existing_table_pair_fn", Type.INT);
+ db.addTableFunction(tableFunction, true);
+
+ Mockito.verify(spyEditLog,
Mockito.never()).logAddFunction(Mockito.any(Function.class));
+ Mockito.verify(spyEditLog,
Mockito.never()).logAddFunctions(Mockito.anyList());
+ Assert.assertSame(existingFunction,
db.getFunction(searchDesc(existingFunction)));
+ Assert.assertSame(existingOuterFunction,
db.getFunction(searchDesc(existingOuterFunction)));
+ } finally {
+ Env.getCurrentEnv().setEditLog(editLog);
+ }
+ }
+
+ @Test
+ public void testCreateTableFunctionLogsPairAtomically() throws Exception {
+ ConnectContext ctx = UtFrameUtils.createDefaultCtx();
+ createDatabase(ctx, "create database atomic_table_function_db;");
+ Database db =
Env.getCurrentInternalCatalog().getDbNullable("atomic_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));
+
Mockito.doNothing().when(spyEditLog).logAddFunctions(Mockito.anyList());
+ Env.getCurrentEnv().setEditLog(spyEditLog);
+ try (MockedStatic<FunctionUtil> mockedFunctionUtil =
Mockito.mockStatic(FunctionUtil.class,
+ Mockito.CALLS_REAL_METHODS)) {
+ mockedFunctionUtil.when(() ->
FunctionUtil.translateToNereidsThrows(
+ Mockito.eq("atomic_table_function_db"),
Mockito.any(Function.class)))
+ .thenReturn(true);
+ Function tableFunction =
createJavaUdtf("atomic_table_function_db", "atomic_table_fn", Type.INT);
+
+ db.addTableFunction(tableFunction, false);
+
+ Mockito.verify(spyEditLog,
Mockito.never()).logAddFunction(Mockito.any(Function.class));
+
Mockito.verify(spyEditLog).logAddFunctions(Mockito.argThat(functions ->
Review Comment:
This only verifies that the live code called the mocked `logAddFunctions()`
method. The PR adds a new durable format (`CreateFunctionInfo` under
`OP_ADD_FUNCTIONS`) plus `JournalEntity.readFields()` and
`EditLog.loadJournal()` replay handling, but none of the new tests write/read
that journal entity or replay it into a fresh catalog. Given this is the fix
for crash/restart atomicity, please add a persistence test that serializes an
`OP_ADD_FUNCTIONS` entry for the base and `_outer` functions, reads it back
through `JournalEntity`, replays it, and asserts both functions are restored.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java:
##########
@@ -769,16 +769,74 @@ 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, false);
+ }
+
+ 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");
+ if (hasSameTableFunctionPair(function, outerFunction, ifNotExists)) {
+ return;
+ }
+ addFunctions(ImmutableList.of(function, outerFunction), false, true);
+ }
+
+ private boolean hasSameTableFunctionPair(Function function, Function
outerFunction, boolean ifNotExists)
+ throws UserException {
+ Function existingFunction = getExistingFunction(function);
+ Function existingOuterFunction = getExistingFunction(outerFunction);
+ if (existingFunction == null && existingOuterFunction == null) {
+ return false;
+ }
+ if (ifNotExists && existingFunction != null && existingOuterFunction
!= null
+ && existingFunction.isUDTFunction() &&
existingOuterFunction.isUDTFunction()) {
+ return true;
+ }
+ throw new UserException("function already exists");
+ }
+
+ private Function getExistingFunction(Function function) {
+ try {
+ return getFunction(getFunctionSearchDesc(function));
+ } catch (AnalysisException e) {
+ return null;
+ }
+ }
+
+ private void addFunctions(List<Function> functions, boolean ifNotExists,
boolean logAsBatch) 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));
Review Comment:
This rollback is still too broad for variadic overloads. Catalog identity
distinguishes fixed and variadic signatures (`FunctionSearchDesc.isIdentical()`
and `Function.isIdentical()` both compare `hasVarArgs`), so an existing
`f(INT...)` can coexist with the fixed `f(INT)` that this failed table-function
batch just added. The cleanup here goes through `dropFromNereids()`, but
`FunctionRegistry.dropUdf()` removes builders by name and `getArgTypes()` only,
ignoring the builder's `hasVarArguments()` flag. If `f(INT)` translates and the
generated `f_outer(INT)` later fails, this line deletes the unrelated variadic
`f(INT...)` builder from the Nereids registry while its catalog entry remains
visible. Please make Nereids UDF removal match the full catalog identity,
including the variadic flag, and add coverage with an existing variadic
overload plus a failed fixed table-function batch.
--
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]
