feat(presto-connector): Add Spotless formatting for the Java plugin. - #39
Open
jackluo923 wants to merge 7 commits into
Open
jackluo923 wants to merge 7 commits into
jackluo923 wants to merge 7 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an Eclipse formatter profile and Spotless Maven configuration for the Presto connector. Taskfiles expose Java check and fix commands, integrate them with shared lint tasks, and document the commands. ChangesJava quality tooling and lint integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Developer
participant Taskfile
participant MavenWrapper
participant Spotless
Developer->>Taskfile: Run check-java or fix-java
Taskfile->>MavenWrapper: Invoke spotless:check or spotless:apply
MavenWrapper->>Spotless: Execute configured Java formatting
Spotless-->>MavenWrapper: Return validation or formatting result
MavenWrapper-->>Taskfile: Return task status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@presto-connector/pom.xml`:
- Around line 320-342: Bind the Spotless check execution in the
spotless-maven-plugin configuration to the Maven validate phase by adding an
explicit phase to the execution containing the check goal. Preserve the existing
formatting configuration, and remove or tolerate any separate lint invocation
that would otherwise duplicate spotless:check.
In
`@presto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpConfig.java`:
- Around line 89-92: Replace each unary negation with the required false
comparison while preserving behavior: update the regex predicate in
ClpConfig.java (89-92), schema-membership predicates in ClpMetadata.java (91 and
107), empty-directory and empty-database-name checks in
ClpMySqlMetadataProvider.java (110-113 and 136-139), and both
polymorphism/type-comparison checks in ClpSchemaTree.java (132-148) to use false
== expression.
In
`@presto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpTableHandle.java`:
- Around line 27-33: Reformat the `@JsonCreator` constructors in ClpTableHandle
(presto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpTableHandle.java,
lines 27-33) and ClpTableLayoutHandle
(presto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpTableLayoutHandle.java,
lines 28-36) so each `@JsonProperty` annotation remains on the same line as its
parameter type and name; alternatively adjust the Eclipse formatter
annotation-wrap setting to preserve this layout for future constructors.
In
`@presto-connector/src/main/java/com/facebook/presto/plugin/clp/codec/ClpColumnHandleCodec.java`:
- Line 48: Replace the negated instanceof checks with the configured
false-comparison style: use false == (handle instanceof ClpColumnHandle) in
ClpColumnHandleCodec, false == (handle instanceof ClpSplit) in ClpSplitCodec,
false == (handle instanceof ClpTableHandle) in ClpTableHandleCodec, and false ==
(handle instanceof ClpTableLayoutHandle) in ClpTableLayoutHandleCodec. Update
the corresponding checks in all four specified files and preserve their existing
behavior.
In
`@presto-connector/src/main/java/com/facebook/presto/plugin/clp/split/filter/ClpMySqlSplitFilterProvider.java`:
- Around line 71-89: Escape the configured column name before interpolating it
into the regex patterns in the mapping loop of ClpMySqlSplitFilterProvider.
Quote key for literal regex matching, then use the escaped value in all three
String.format patterns so names containing metacharacters such as "." cannot
match unintended SQL text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a115c1e-f307-4cf6-ad66-c62fd624b73a
📒 Files selected for processing (59)
README.mdpresto-connector/.eclipse-formatter.xmlpresto-connector/pom.xmlpresto-connector/spotbugs-exclude.xmlpresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpColumnHandle.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpConfig.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpConnector.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpConnectorFactory.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpErrorCode.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpExpression.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpFunctions.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpHandleResolver.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpMetadata.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpModule.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpPlugin.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpRecordSetProvider.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpSplit.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpSplitManager.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpTableHandle.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpTableLayoutHandle.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/ClpTransactionHandle.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/codec/ClpColumnHandleCodec.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/codec/ClpConnectorCodecProvider.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/codec/ClpSplitCodec.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/codec/ClpTableHandleCodec.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/codec/ClpTableLayoutHandleCodec.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/codec/ClpTransactionHandleCodec.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/codec/CodecUtils.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMySqlMetadataProvider.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpSchemaTree.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpSchemaTreeNodeType.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpComputePushDown.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpPlanOptimizerProvider.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpUdfRewriter.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/split/ClpMySqlSplitProvider.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/split/ClpSplitProvider.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/split/filter/ClpMySqlSplitFilterProvider.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/split/filter/ClpSplitFilterConfig.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/split/filter/ClpSplitFilterConfigCustomOptionsDeserializer.javapresto-connector/src/main/java/com/facebook/presto/plugin/clp/split/filter/ClpSplitFilterProvider.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/ClpMetadataDbSetUp.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/TestClpMetadata.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/TestClpQueryBase.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/TestClpSplit.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/TestClpUdfRewriter.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/codec/TestClpConnectorCodecProvider.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/mockdb/ClpMockMetadataDatabase.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/mockdb/table/ArchivesTableRows.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/mockdb/table/ColumnMetadataTableRows.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/mockdb/table/DatasetsTableRows.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/split/filter/TestClpMySqlSplitFilterConfig.javapresto-connector/src/test/java/com/facebook/presto/plugin/clp/split/filter/TestClpSplitFilterConfigCommon.javataskfiles/lint.yamltaskfiles/presto-connector/lint.yamltaskfiles/presto-connector/main.yaml
jackluo923
force-pushed
the
feat/presto-connector-linting
branch
from
July 30, 2026 23:56
ed76f58 to
c053eba
Compare
jackluo923
force-pushed
the
feat/presto-connector-linting
branch
from
July 31, 2026 00:15
c053eba to
fa36d8d
Compare
jackluo923
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
presto-connector/pom.xml (1)
319-324: 🎯 Functional Correctness | 🟠 MajorAdd the missing
<phase>validate</phase>binding.The surrounding comment says
phase:validate, but thecheckexecution at Lines 320–324 has no<phase>. Formatting therefore is not enforced during the Mavenvalidatelifecycle; the direct Taskfile invocation only masks this gap. This is the same unresolved issue raised in the previous review.Proposed fix
<execution> + <phase>validate</phase> <goals> <goal>check</goal> </goals> </execution>#!/usr/bin/env bash set -euo pipefail python3 - <<'PY' import xml.etree.ElementTree as ET ns = {"m": "http://maven.apache.org/POM/4.0.0"} pom = ET.parse("presto-connector/pom.xml") for execution in pom.findall( ".//m:plugin[m:artifactId='spotless-maven-plugin']/m:executions/m:execution", ns, ): goals = { goal.text for goal in execution.findall("./m:goals/m:goal", ns) } if "check" in goals: assert execution.findtext("m:phase", namespaces=ns) == "validate" break else: raise SystemExit("Spotless check execution not found") PY🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@presto-connector/pom.xml` around lines 319 - 324, Add the missing validate lifecycle binding to the Spotless execution containing the check goal in the Maven configuration. Update that execution, rather than nearby goals or plugins, so its phase is validate and formatting checks run during Maven validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@presto-connector/pom.xml`:
- Around line 319-324: Add the missing validate lifecycle binding to the
Spotless execution containing the check goal in the Maven configuration. Update
that execution, rather than nearby goals or plugins, so its phase is validate
and formatting checks run during Maven validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8198dcb5-5ac3-4ad6-9c5e-896a5b73eb40
📒 Files selected for processing (6)
README.mdpresto-connector/.eclipse-formatter.xmlpresto-connector/pom.xmltaskfiles/lint.yamltaskfiles/presto-connector/lint.yamltaskfiles/presto-connector/main.yaml
jackluo923
force-pushed
the
feat/presto-connector-linting
branch
from
July 31, 2026 00:37
d9baf86 to
848e61e
Compare
jackluo923
changed the title
jackluo923
force-pushed
the
feat/presto-connector-linting
branch
2 times, most recently
from
July 31, 2026 15:06
278e731 to
0993d3c
Compare
jackluo923
force-pushed
the
feat/presto-connector-linting
branch
from
July 31, 2026 23:21
0993d3c to
3149f5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@presto-connector/.eclipse-formatter.xml`:
- Line 132: Update the Eclipse formatter settings so both
org.eclipse.jdt.core.formatter.indentation.size and
org.eclipse.jdt.core.formatter.tabulation.size use value 2, ensuring the
presto-connector formatting style uses two-space indentation.
In `@README.md`:
- Around line 37-42: Update the README lint command documentation to clarify
that the aggregate lint:check currently runs only YAML checks; explicitly
instruct developers to run lint:check-java separately, and state that Java
checking is not enforced by the aggregate command in this commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ecb75aa8-9163-4c7b-ad9c-d184368254e8
📒 Files selected for processing (6)
README.mdpresto-connector/.eclipse-formatter.xmlpresto-connector/pom.xmltaskfiles/lint.yamltaskfiles/presto-connector/lint.yamltaskfiles/presto-connector/main.yaml
| <setting id="org.eclipse.jdt.core.formatter.indent_statements_compare_to_body" value="true"/> | ||
| <setting id="org.eclipse.jdt.core.formatter.indent_switchstatements_compare_to_cases" value="true"/> | ||
| <setting id="org.eclipse.jdt.core.formatter.indent_switchstatements_compare_to_switch" value="true"/> | ||
| <setting id="org.eclipse.jdt.core.formatter.indentation.size" value="4"/> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import xml.etree.ElementTree as ET
path = "presto-connector/.eclipse-formatter.xml"
settings = {
node.attrib["id"]: node.attrib["value"]
for node in ET.parse(path).findall(".//setting")
}
assert settings["org.eclipse.jdt.core.formatter.indentation.size"] == "2"
assert settings["org.eclipse.jdt.core.formatter.tabulation.size"] == "2"
PYRepository: y-scope/clp-plugin-presto-connector
Length of output: 261
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import xml.etree.ElementTree as ET
from pathlib import Path
path = Path("presto-connector/.eclipse-formatter.xml")
assert path.exists()
tree = ET.parse(path)
root = tree.getroot()
ns = {"j": "http://www.eclipse.org/jdt/core/formatter"}
settings = {node.attrib["id"]: node.attrib["value"] for node in root.findall(".//setting")}
print("indentation.size:", settings.get("org.eclipse.jdt.core.formatter.indentation.size"))
print("tabulation.size:", settings.get("org.eclipse.jdt.core.formatter.tabulation.size"))
print("tabulation.char:", settings.get("org.eclipse.jdt.core.formatter.tabulation.char"))
for node in root.findall(".//setting"):
if node.attrib["id"] in {
"org.eclipse.jdt.core.formatter.indentation.size",
"org.eclipse.jdt.core.formatter.tabulation.size",
"org.eclipse.jdt.core.formatter.tabulation.char",
}:
print(f"{node.attrib['id']}={node.attrib['value']}")
PYRepository: y-scope/clp-plugin-presto-connector
Length of output: 387
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import xml.etree.ElementTree as ET
path = "presto-connector/.eclipse-formatter.xml"
settings = {
node.attrib["id"]: node.attrib["value"]
for node in ET.parse(path).findall(".//setting")
}
assert settings["org.eclipse.jdt.core.formatter.indentation.size"] == "2"
assert settings["org.eclipse.jdt.core.formatter.tabulation.size"] == "2"
PYRepository: y-scope/clp-plugin-presto-connector
Length of output: 261
Set the Eclipse formatter to two-space indentation.
presto-connector/.eclipse-formatter.xml uses space-based indentation, but both org.eclipse.jdt.core.formatter.indentation.size and org.eclipse.jdt.core.formatter.tabulation.size are set to 4. Set both values to 2 for the required two-space style.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@presto-connector/.eclipse-formatter.xml` at line 132, Update the Eclipse
formatter settings so both org.eclipse.jdt.core.formatter.indentation.size and
org.eclipse.jdt.core.formatter.tabulation.size use value 2, ensuring the
presto-connector formatting style uses two-space indentation.
davidlion
reviewed
Aug 1, 2026
| <artifactId>spotless-maven-plugin</artifactId> | ||
| <configuration> | ||
| <java> | ||
| <cleanthat/> |
Member
There was a problem hiding this comment.
We should add a comment on why we need "cleanthat" as it is unclear what its purpose is. This will be useful for a future dev so they don't mistakenly decide we need to keep or remove it.
If they are not covered by another formatter we should also add the generic "trim white space" and "end with newline" steps.
Member
Author
There was a problem hiding this comment.
Added a comment explaining CleanThat in caad2dc. It's the one step in the java block that rewrites code rather than reformatting it — it applies CleanThat's default SafeAndConsensual mutators, e.g. values.size() == 0 becomes values.isEmpty() — so the bare <cleanthat/> element doesn't convey what dropping it would cost.
On trim-whitespace and end-with-newline: they're already covered, so I haven't added them. I tested a file with trailing whitespace on code lines, in javadoc, in a block comment and in a line comment, plus no final newline, with <cleanthat/> removed so the <eclipse> step was isolated — it stripped every trailing space and added the newline. Both steps would be no-ops here.
One thing worth deciding separately: those steps live inside <java>, so they'd only ever cover .java files. If the goal is repo-wide whitespace hygiene (pom.xml, markdown, taskfiles), that needs a separate <formats> block — happy to add it if you want that.
jackluo923
force-pushed
the
feat/presto-connector-linting
branch
from
August 2, 2026 04:27
3149f5b to
91d9f83
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
presto-connector/.eclipse-formatter.xml (1)
132-132:⚠️ Potential issue | 🟠 MajorSet both indentation sizes to
2.
org.eclipse.jdt.core.formatter.indentation.sizeandorg.eclipse.jdt.core.formatter.tabulation.sizeare both set to4. This does not implement the required two-space style. Set both values to2.Proposed formatter fix
- <setting id="org.eclipse.jdt.core.formatter.indentation.size" value="4"/> + <setting id="org.eclipse.jdt.core.formatter.indentation.size" value="2"/> ... - <setting id="org.eclipse.jdt.core.formatter.tabulation.size" value="4"/> + <setting id="org.eclipse.jdt.core.formatter.tabulation.size" value="2"/>Also applies to: 386-386
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@presto-connector/.eclipse-formatter.xml` at line 132, Update the Eclipse formatter settings for org.eclipse.jdt.core.formatter.indentation.size and org.eclipse.jdt.core.formatter.tabulation.size to use value 2 instead of 4.README.md (1)
37-42:⚠️ Potential issue | 🟡 MinorClarify that Java checking is separate in this commit.
task lint:checkstill runs only the YAML check. Developers must runtask lint:check-javaseparately until Java checking is added to the aggregate check. Update the text above this table to state this limitation.Proposed documentation update
-The commands above run all linting checks, but for performance you may want to run a subset using -one of the tasks in the table below. +`task lint:check` runs YAML checks only in this commit. Run `task lint:check-java` separately. +For performance, you may want to run a subset using one of the tasks in the table below.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 37 - 42, Update the documentation immediately above the task table to state that task lint:check currently runs only the YAML check and Java formatting must be checked separately with task lint:check-java until Java checking is added to the aggregate command.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@presto-connector/.eclipse-formatter.xml`:
- Line 132: Update the Eclipse formatter settings for
org.eclipse.jdt.core.formatter.indentation.size and
org.eclipse.jdt.core.formatter.tabulation.size to use value 2 instead of 4.
In `@README.md`:
- Around line 37-42: Update the documentation immediately above the task table
to state that task lint:check currently runs only the YAML check and Java
formatting must be checked separately with task lint:check-java until Java
checking is added to the aggregate command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 18db2e70-cfdf-4c3c-bc60-c9467f018e17
📒 Files selected for processing (6)
README.mdpresto-connector/.eclipse-formatter.xmlpresto-connector/pom.xmltaskfiles/lint.yamltaskfiles/presto-connector/lint.yamltaskfiles/presto-connector/main.yaml
9 tasks done
jackluo923
force-pushed
the
feat/presto-connector-linting
branch
from
August 4, 2026 23:45
caad2dc to
5245e0f
Compare
Ports the Spotless half of #7 onto current main: the Eclipse formatter profile is taken from #7 (LF-normalized; same plugin version), while the pom and taskfile wiring is redone against main's restructured build (mvnw, taskfiles/presto-connector/). This commit ships the complete mechanism but runs none of it: both lint:fix-java (spotless:apply, included in lint:fix) and lint:check-java (spotless:check) are defined and available to invoke by hand, yet nothing calls the check automatically -- no CI workflow, no Maven phase binding, and no inclusion in the aggregate lint:check. The sources are still unformatted here, so lint:check-java fails if invoked; that is expected and harmless. The boundary is deliberate: lint:check defines what the repo enforces, so check-java joins it only in the follow-up, atomically with the mechanical reformat -- ensuring no commit exists where the check is enforced but the sources are unformatted, and every commit stays green under its own rules.
CleanThat is the only step in the `java` block that rewrites code rather than reformatting it, which isn't apparent from the bare `<cleanthat/>` element.
jackluo923
force-pushed
the
feat/presto-connector-linting
branch
from
August 8, 2026 20:30
5245e0f to
2ec16cf
Compare
The private fork runs CI on self-hosted hardware, which the hardcoded `ubuntu-24.04` and `ubuntu-24.04-arm` labels gave it no way to select. Every job now reads its label set from a repository variable, matching the convention already used in clp-plugin-presto-velox. Jobs that compile Velox or build images take the heavy variables; the rest take the light ones. `integration-tests` counts as heavy because it builds the connector before it starts a cluster, which is the bulk of its runtime. `pr-title-checks` moves off `ubuntu-latest` so no job is left unswitchable. Each expression falls back to the label the job used before, so the public repository and pull requests from forks behave exactly as they did when the variables are unset. `build-packages.sh` refused to run as root so that `sudo` would not leave the staging directories and artifacts owned by root. That also rejected the self-hosted runners, whose agent runs as root legitimately and is therefore the intended owner. The check now fires only when `SUDO_USER` is set, which is the case its message describes.
The private fork runs CI on self-hosted hardware, which the hardcoded `ubuntu-24.04` and `ubuntu-24.04-arm` labels gave it no way to select. Every job now reads its label set from a repository variable, matching the convention already used in clp-plugin-presto-velox. Jobs that compile Velox or build images take the heavy variables; the rest take the light ones. `integration-tests` counts as heavy because it builds the connector before it starts a cluster, which is the bulk of its runtime. `pr-title-checks` moves off `ubuntu-latest` so no job is left unswitchable. Each expression falls back to the label the job used before, so the public repository and pull requests from forks behave exactly as they did when the variables are unset. Running `integration-tests` on a self-hosted runner exposed two assumptions in `build-packages.sh` that only held on GitHub-hosted runners. It refused to run as root so that `sudo` would not leave the staging directories and artifacts owned by root, which also rejected an agent that runs as root legitimately and is therefore the intended owner; the check now fires only when `SUDO_USER` is set, which is the case its message describes. It also staged container output under `/tmp`, which a containerized runner's Docker daemon cannot bind-mount because the path exists only in the job's own mount namespace; staging now sits under the source tree, which is bind-mounted into the container either way. The tests themselves now run in a container on the cluster's network rather than against a published port, because a published port is reachable only from a process that shares a network namespace with the Docker daemon, which a containerized runner does not. The cluster's lifecycle moves from the `client` fixture to `integration-tests:up` and `:down`, so the harness no longer runs `docker compose` and pytest keeps working unchanged from the host, against the port that is still published for that purpose. CI gives each run its own compose project and an arbitrary host port, because a machine can run more than one job at a time, and stops the cluster in its own step now that a runner outlives the job.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds Spotless formatting for the Java plugin — the complete mechanism (configuration, fix and check tooling), but nothing that runs the check:
presto-connector/pom.xml: adds thespotless-maven-plugin(3.5.1), configured with the Eclipse formatter profile, import ordering, and unused-import removal. Nocheckexecution bound to any build phase.presto-connector/.eclipse-formatter.xml: Eclipse formatter profile (from log4j2-appenders, with 2-space indentation).lint:fix-java(spotless:apply) included inlint:fix, andlint:check-java(spotless:check) — defined but not included inlint:check.README.md: documents the new lint tasks.Note that this PR deliberately does not format the sources or enforce the check. Nothing at this commit (CI,
lint:check, or the Maven build) runs the check. The follow-up stacked on top (#41) commits the auto-formatting output and turns on enforcement atomically.Breaking changes
None
Validation performed
task lint:check-javafails on the unformatted sources (expected); aftertask lint:fix-java, it passes with no errors. The formatting diff was then discarded, to be committed in feat(presto-connector): Format all Java sources with Spotless and enforce it. #41.yamllint(repo config) passes on the new and changed taskfiles.Summary by CodeRabbit
New Features
Documentation
Style