Skip to content

Conversation

@Cpaulyz
Copy link
Contributor

@Cpaulyz Cpaulyz commented Nov 19, 2024

Description

  1. Refactor UDF management process. Most important change is to introduce an enum type Model. UDFTable use Pair<Model, String> as hash key to support table model user-defined function now.
  2. Introduce pre create and pre drop process to avoid some corner cases due to partial create/drop. Function with UNAVAILABLE state in SHOW FUNCTIONS will perform some unexpected behaviors.

See https://timechor.feishu.cn/docx/TCyYdm8pYowe0HxEGb9c2UfenCf for more details.

@codecov
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 27.25061% with 299 lines in your changes missing coverage. Please review.

Project coverage is 39.64%. Comparing base (b8432f8) to head (54b24c2).
Report is 128 commits behind head on master.

Files with missing lines Patch % Lines
...n/execution/config/metadata/ShowFunctionsTask.java 0.00% 50 Missing ⚠️
...rg/apache/iotdb/confignode/manager/UDFManager.java 0.00% 49 Missing ⚠️
...otdb/commons/udf/service/UDFManagementService.java 33.33% 42 Missing ⚠️
...otdb/commons/udf/service/UDFExecutableManager.java 0.00% 25 Missing ⚠️
...ion/config/executor/ClusterConfigTaskExecutor.java 0.00% 21 Missing ⚠️
...sus/request/write/function/UpdateFunctionPlan.java 0.00% 17 Missing ⚠️
...g/apache/iotdb/confignode/persistence/UDFInfo.java 0.00% 14 Missing ⚠️
.../execution/config/metadata/CreateFunctionTask.java 0.00% 13 Missing ⚠️
...in/java/org/apache/iotdb/commons/udf/UDFTable.java 66.66% 10 Missing ⚠️
...ain/java/org/apache/iotdb/db/service/DataNode.java 0.00% 9 Missing ⚠️
... and 16 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14135      +/-   ##
============================================
- Coverage     39.95%   39.64%   -0.32%     
  Complexity       71       71              
============================================
  Files          4191     4238      +47     
  Lines        265828   270193    +4365     
  Branches      32265    32777     +512     
============================================
+ Hits         106217   107122     +905     
- Misses       159611   163071    +3460     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@OneSizeFitsQuorum
Copy link
Contributor

What a spectacular work 🦾! Fabulous design and implementation⚡️⚡️🔥


@Override
protected void deserializeImpl(ByteBuffer buffer) throws IOException {
model = Model.findByValue(ReadWriteIOUtils.readInt(buffer));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't be compatible to previous version

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may need to add a new PlanType

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Split it into DropTableModelFunctionPlan and DropTreeModelFunctionPlan

CreateFunctionPlan createFunctionPlan =
new CreateFunctionPlan(udfInformation, needToSaveJar ? new Binary(jarFile) : null);

udfInformation.setAvailable(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true or false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be false. Fixed.

Comment on lines 44 to 48
/** functionName -> information * */
private final Map<Pair<Model, String>, UDFInformation> udfInformationMap;

/** maintain a map for creating instance, functionName -> class */
private final Map<Pair<Model, String>, Class<?>> functionToClassMap;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using Map<Model, Map<String, UDFInformation>>? In this way, you won't need to iterate all map entry while trying to all functions of one kind of model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Use Map<Model, Map<String, UDFInformation>> now

TREE_EXTERNAL((byte) 0),
TREE_BUILT_IN((byte) 1),
TREE_UNAVAILABLE((byte) 2),
TABLE_EXTERNAL((byte) 3),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TABLE_EXTERNAL((byte) 3),
TABLE_AVAILABLE((byte) 3),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

import java.nio.ByteBuffer;

public enum UDFType {
TREE_EXTERNAL((byte) 0),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TREE_EXTERNAL((byte) 0),
TREE_AVAILABLE((byte) 0),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


public enum UDFType {
TREE_EXTERNAL((byte) 0),
TREE_BUILT_IN((byte) 1),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some comments for this enum to explain that this won't appear in previous snapshot file or raft log, just a place holder for some unforeseen circumstances.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
11.0% Duplication on New Code (required ≤ 5%)

See analysis details on SonarQube Cloud

@JackieTien97 JackieTien97 merged commit d751364 into master Nov 27, 2024
40 of 43 checks passed
@JackieTien97 JackieTien97 deleted the table_udsf branch November 27, 2024 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants