Copilot commented on code in PR #155: URL: https://github.com/apache/hbase-connectors/pull/155#discussion_r3320325229
########## spark/hbase-spark-pushdown/pom.xml: ########## @@ -0,0 +1,221 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under this License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + +--> +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.hbase.connectors</groupId> + <artifactId>spark</artifactId> + <version>${revision}</version> + <relativePath>../pom.xml</relativePath> + </parent> + + <groupId>org.apache.hbase.connectors.spark</groupId> + <artifactId>hbase-spark-pushdown_${scala.binary.version}</artifactId> + <name>Apache HBase - Spark Connector SQL Pushdown (shared)</name> + <description>Spark-version-neutral SQL pushdown for HBase: SparkSQLPushDownFilter, dynamic logic AST, BytesEncoder façade. + Built once per Scala line (Spark 3 / Spark 4 reactors share sources).</description> + + <dependencies> + <dependency> + <groupId>org.slf4j</groupId> + <artifactId>slf4j-api</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase.thirdparty</groupId> + <artifactId>hbase-shaded-miscellaneous</artifactId> + </dependency> + <dependency> + <groupId>org.scala-lang</groupId> + <artifactId>scala-library</artifactId> + <version>${scala.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.spark</groupId> + <artifactId>spark-core_${scala.binary.version}</artifactId> + <version>${spark.version}</version> + <scope>provided</scope> + <exclusions> + <exclusion> + <groupId>org.scala-lang</groupId> + <artifactId>scala-library</artifactId> + </exclusion> + <exclusion> + <groupId>org.scala-lang</groupId> + <artifactId>scalap</artifactId> + </exclusion> + <exclusion> + <groupId>com.google.code.findbugs</groupId> + <artifactId>jsr305</artifactId> + </exclusion> + <exclusion> + <groupId>org.xerial.snappy</groupId> + <artifactId>snappy-java</artifactId> + </exclusion> + <exclusion> + <groupId>xerces</groupId> + <artifactId>xercesImpl</artifactId> + </exclusion> + <exclusion> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-client-api</artifactId> + </exclusion> + <exclusion> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-client-runtime</artifactId> + </exclusion> + </exclusions> + </dependency> + <dependency> + <groupId>org.apache.spark</groupId> + <artifactId>spark-sql_${scala.binary.version}</artifactId> + <version>${spark.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-client</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-common</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.hbase.connectors.spark</groupId> + <artifactId>hbase-spark-protocol-shaded</artifactId> + </dependency> + <dependency> + <groupId>org.apache.yetus</groupId> + <artifactId>audience-annotations</artifactId> + </dependency> + </dependencies> + + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + </plugin> + <plugin> + <groupId>org.codehaus.mojo</groupId> + <artifactId>build-helper-maven-plugin</artifactId> + <executions> + <execution> + <id>add-java-source-dir</id> + <goals> + <goal>add-source</goal> + </goals> + <phase>generate-sources</phase> + <configuration> + <sources> + <source>src/main/java</source> + </sources> + </configuration> + </execution> + <execution> + <id>add-scala-source-dir</id> + <goals> + <goal>add-source</goal> + </goals> + <phase>validate</phase> + <configuration> + <sources> + <source>src/main/scala</source> + </sources> + </configuration> + </execution> + </executions> + </plugin> Review Comment: The new `hbase-spark-pushdown` module is declared (added to `spark/pom.xml` as a module and as a dependency of `hbase-spark`), but the directory `spark/hbase-spark-pushdown/` only contains a `pom.xml` — there are no `src/main/java` or `src/main/scala` directories. The actual pushdown sources (`SparkSQLPushDownFilter.java`, `DynamicLogicExpression`, etc.) still live under `spark/hbase-spark/src/main/...`. As a result this module will build an empty jar, the dependency declared in `spark/hbase-spark/pom.xml` provides nothing, and the assembly will ship an empty `hbase-spark-pushdown_2.12.jar`. If the intent of the PR is to factor pushdown out as a shared module, the source files need to be moved into `spark/hbase-spark-pushdown/src/...` (and removed from `spark/hbase-spark`). If the move is intentionally deferred, the module/dependency wiring should also be deferred so we don't ship an empty artifact. ########## spark4/hbase-spark-pushdown/pom.xml: ########## @@ -0,0 +1,218 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under this License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + +--> +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.hbase.connectors</groupId> + <artifactId>spark4</artifactId> + <version>${revision}</version> + <relativePath>../pom.xml</relativePath> + </parent> + + <groupId>org.apache.hbase.connectors.spark</groupId> + <artifactId>hbase-spark-pushdown_${scala.binary.version}</artifactId> + <name>Apache HBase - Spark Connector SQL Pushdown (shared for Spark 4)</name> + <description> + Builds the same pushdown sources as spark/hbase-spark-pushdown against Spark 4 / Scala 2.13 (see module sources). + </description> + + <dependencies> + <dependency> + <groupId>org.slf4j</groupId> + <artifactId>slf4j-api</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase.thirdparty</groupId> + <artifactId>hbase-shaded-miscellaneous</artifactId> + </dependency> + <dependency> + <groupId>org.scala-lang</groupId> + <artifactId>scala-library</artifactId> + <version>${scala.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.spark</groupId> + <artifactId>spark-core_${scala.binary.version}</artifactId> + <version>${spark4.version}</version> + <scope>provided</scope> + <exclusions> + <exclusion> + <groupId>org.scala-lang</groupId> + <artifactId>scala-library</artifactId> + </exclusion> + <exclusion> + <groupId>org.scala-lang</groupId> + <artifactId>scalap</artifactId> + </exclusion> + <exclusion> + <groupId>com.google.code.findbugs</groupId> + <artifactId>jsr305</artifactId> + </exclusion> + <exclusion> + <groupId>org.xerial.snappy</groupId> + <artifactId>snappy-java</artifactId> + </exclusion> + <exclusion> + <groupId>xerces</groupId> + <artifactId>xercesImpl</artifactId> + </exclusion> + <exclusion> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-client-api</artifactId> + </exclusion> + <exclusion> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-client-runtime</artifactId> + </exclusion> + </exclusions> + </dependency> + <dependency> + <groupId>org.apache.spark</groupId> + <artifactId>spark-sql_${scala.binary.version}</artifactId> + <version>${spark4.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-client</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-common</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>org.apache.hbase.connectors.spark</groupId> + <artifactId>hbase-spark-protocol-shaded</artifactId> + <version>${revision}</version> + </dependency> + <dependency> + <groupId>org.apache.yetus</groupId> + <artifactId>audience-annotations</artifactId> + </dependency> + </dependencies> + + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + </plugin> + <plugin> + <groupId>org.codehaus.mojo</groupId> + <artifactId>build-helper-maven-plugin</artifactId> + <executions> + <execution> + <id>add-shared-pushdown-sources-java</id> + <phase>validate</phase> + <goals> + <goal>add-source</goal> + </goals> + <configuration> + <sources> + <source>${project.basedir}/../../spark/hbase-spark-pushdown/src/main/java</source> + <source>${project.basedir}/../../spark/hbase-spark-pushdown/src/main/scala</source> Review Comment: The `add-shared-pushdown-sources-java` execution adds `${project.basedir}/../../spark/hbase-spark-pushdown/src/main/java` and `.../src/main/scala` as source roots, but those directories do not exist in this PR (only `spark/hbase-spark-pushdown/pom.xml` was created — no `src/`). The Spark 4 pushdown module will therefore compile nothing and produce an empty jar. Either the shared pushdown sources need to be relocated to `spark/hbase-spark-pushdown/src/...` as part of this PR, or these `<source>` entries need to point at the location where the sources actually live today (e.g. `spark/hbase-spark/src/main/{java,scala}` filtered to just the pushdown classes). ########## spark/hbase-spark-pushdown/pom.xml: ########## @@ -0,0 +1,221 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under this License is distributed on an "AS IS" BASIS, Review Comment: The ALv2 boilerplate is mangled here: the standard text reads `Unless required by applicable law or agreed to in writing, software` / `distributed under the License is distributed on an "AS IS" BASIS,`. This file has `writing,` on one line and then `software distributed under this License is distributed on an "AS IS" BASIS,` ("this License" instead of "the License", and the line break in the wrong place). Please restore the standard wording — compare e.g. `spark/pom.xml` lines 15–19 or `hbase-connectors-assembly/src/main/assembly/hbase-connectors-bin.xml` lines 17–18. The same wording bug is present in `spark4/hbase-spark-pushdown/pom.xml` lines 13–14. ########## spark4/hbase-spark4/src/main/scala/org/apache/hadoop/hbase/spark/Spark4Placeholder.scala: ########## @@ -0,0 +1,28 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hbase.spark + +import org.apache.yetus.audience.InterfaceAudience + +/** + * Phase-0 build marker for the Spark 4 connector line (`hbase-spark4`). + * Replaced incrementally during feature porting; see ../../SPARK4-ROADMAP.md at repo root. Review Comment: The comment refers to `../../SPARK4-ROADMAP.md at repo root`, but no such file exists anywhere in the repository (searched for `SPARK4-ROADMAP`, no matches). The same file is also referenced from `spark4/hbase-spark4/pom.xml` (`<description>` on line 34) and `hbase-connectors-assembly/src/main/assembly/hbase-connectors-bin.xml` (line 38). Either add the roadmap document to the repo, or drop these references to avoid pointing readers at a non-existent file. ########## hbase-connectors-assembly/src/main/assembly/hbase-connectors-bin.xml: ########## @@ -34,6 +34,10 @@ <useAllReactorProjects>true</useAllReactorProjects> <excludes> <exclude>org.apache.hbase.connectors.spark:hbase-spark-it</exclude> + <!-- Not yet packaged when assembly runs (reactor lists spark4 after this module). + Exclude until dist layout includes Spark 4; see SPARK4-ROADMAP.md --> + <exclude>org.apache.hbase.connectors.spark:hbase-spark4</exclude> Review Comment: The exclusion for the Spark 4 pushdown jar hard-codes the Scala binary suffix `_2.13`. The artifactId in `spark4/hbase-spark-pushdown/pom.xml` is `hbase-spark-pushdown_${scala.binary.version}`; if/when `scala.binary.version` is bumped (e.g. to a future 3.x line), this exclusion will silently stop matching and the jar will be packaged unintentionally. Consider keying the exclude off the property (e.g. by deriving it the same way as the artifactId) or adding a comment that this string must be updated in lockstep with `scala.binary.version` in `spark4/pom.xml`. ########## pom.xml: ########## @@ -1009,6 +1009,19 @@ ${jacocoArgLine}</argLine> </properties> </profile> + <profile> + <!-- Apache Spark 4.x connector line (JDK 17+). Not included in default CI; activates spark4 reactor. --> + <id>spark4</id> + <activation> + <activeByDefault>false</activeByDefault> + </activation> + <modules combine.children="override"> + <module>kafka</module> + <module>spark</module> + <module>spark4</module> + <module>hbase-connectors-assembly</module> Review Comment: The `spark4` profile uses `<modules combine.children="override">` to redefine the reactor as `kafka`, `spark`, `spark4`, `hbase-connectors-assembly`. This silently drops `test-reporting` (declared in the default `<modules>` of the root pom), so running `mvn ... -Pspark4 -Pcoverage` will no longer aggregate coverage. If that's intentional, consider documenting it; otherwise include `test-reporting` here so the profile is additive rather than subtractive for non-spark modules. ########## spark4/hbase-spark-pushdown/pom.xml: ########## @@ -0,0 +1,218 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under this License is distributed on an "AS IS" BASIS, Review Comment: Same broken ALv2 boilerplate as in `spark/hbase-spark-pushdown/pom.xml`: `writing,` is on a separate line and the next line reads `software distributed under this License…` instead of the standard `…software` / `distributed under the License…`. Please restore the canonical wording used elsewhere in this PR (e.g. `spark4/pom.xml` and `spark4/hbase-spark4/pom.xml`). -- 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]
