DaZuiZui commented on PR #18006: URL: https://github.com/apache/iotdb/pull/18006#issuecomment-4920936016
我本地看了一轮这个 PR，暂时没有发现需要阻塞合并的功能性 bug。 重点看了 FFT table function 的 analyzer / planner / distributed plan / runtime processor 几块： - `SAMPLE_INTERVAL`、`N`、`NORM`、`TIMECOL` 的参数校验比较完整，`SAMPLE_INTERVAL` 指定时要求 duration literal，默认时按 partition 时间范围推断。 - `N` 有 65,536 上限，同时也限制了 `N * 2 * valueColumnCount` 的 spectrum buffer，未指定 `N` 时也会逐行检查，避免默认模式无限缓存。 - null numeric value、非严格递增时间、无 numeric column、错误 order by 这些异常路径都有覆盖。 - 分布式计划里 FFT 没有被拆到各 child 上分别计算，而是通过 `MergeSort(exchange...)` 汇总后再处理，这点看起来是对的。 - LICENSE-binary 里新增的 JTransforms / JLargeArrays / commons-math3 与 Maven 解析出来的依赖版本一致。 我本地跑过： ```bash ./mvnw test -pl iotdb-core/node-commons -Dtest=FFTTableFunctionTest ./mvnw test -pl iotdb-core/datanode -Dtest=TableFunctionTest ./mvnw -pl iotdb-core/node-commons dependency:tree -Dincludes=com.github.wendykierp:JTransforms,pl.edu.icm:JLargeArrays,org.apache.commons:commons-math3 ``` 前两个测试都通过，dependency tree 也确认了新增依赖版本。 有两个非阻塞质量建议： 1. 这个 PR 的主线是 FFT table function，但里面也带了 C++ Session close / tablet bounds 相关改动。代码方向本身没问题，不过和 FFT 功能关联不强，后续最好拆单独 PR，或者至少在 PR 描述里解释一下为什么一起改，方便 review 和回归追踪。 2. FFT 目前和 M4 一样，在 `StatementAnalyzer`、`RelationPlanner`、`TableDistributedPlanGenerator` 里有一些 function-name based special case。当前可以接受，但如果后续继续增加类似“由 proper schema 自己输出 partition columns”的 TVF，建议把这类能力做成 table function analysis 的显式属性，避免 hardcode 继续扩散。 我也尝试跑新增的单个 IT： ```bash ./mvnw verify -DskipUTs -Dit.test=IoTDBFFTTableFunctionIT -DfailIfNoTests=false -Dfailsafe.failIfNoSpecifiedTests=false -pl integration-test -PTableSimpleIT -P with-integration-tests ``` 但本地在 integration-test 编译阶段失败，错误是 `AbstractEnv.java` 里找不到 `org.apache.iotdb.jdbc.Config.SSL_PROTOCOL`，看起来像本地 snapshot / cache 依赖不一致，不是这个 PR 新增 FFT 代码直接导致的。 -- 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]
