bitflicker64 commented on code in PR #785:
URL:
https://github.com/apache/hugegraph-toolchain/pull/785#discussion_r4207006481
##########
.github/actions/setup-candidate-sdk/action.yml:
##########
@@ -6,11 +6,11 @@ inputs:
description: 'GitHub owner/repository for the candidate SDK'
default: 'apache/hugegraph'
ref:
- description: 'Fetchable candidate source ref'
- default: 'd9abcd4317fb36128e7e4d209139ec7f3a28cdfc'
+ description: 'Immutable candidate source commit'
+ default: 'd9abcd'
Review Comment:
Important: The SDK this default selects makes the PR's own required gate
fail. At 15fe93f both `spark-loader-api (candidate)` and `spark-loader-api
(release-1.7)` fail with `Spark Loader artifact check failed:
fixture-verify-vertices.log exited 1, expected 0`. The cause is in the locked
SDK: `AbstractRestClient.buildRequestBody` at `d9abcd43` calls
`bodyContent.getBytes()` with no charset. On a Java 17 executor whose default
charset is US-ASCII, the non-ASCII vertex id is sent as `??` and stored under
the wrong id, with no error. The PR body links this to apache/hugegraph#3281,
but merging now ships a Spark API path that silently corrupts non-ASCII ids and
properties under such a locale. Please lock to an ASF commit whose request body
uses `getBytes(StandardCharsets.UTF_8)`, or set `-Dfile.encoding=UTF-8` for the
driver and executors in the launcher. Keep this gate required and green before
merge.
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/spark/HugeGraphSparkLoader.java:
##########
@@ -282,25 +289,29 @@ private Dataset<Row> read(SparkSession ss, InputStruct
struct) {
return ds;
}
- private void parse(Row row, Map.Entry<ElementBuilder, List<GraphElement>>
builderMap,
+ private static void parse(Row row, Map.Entry<ElementBuilder,
List<GraphElement>> builderMap,
InputStruct struct) {
ElementBuilder builder = builderMap.getKey();
List<GraphElement> graphElements = builderMap.getValue();
- if ("".equals(row.mkString())) {
- return;
- }
List<GraphElement> elements;
switch (struct.input().type()) {
case FILE:
case HDFS:
FileSource fileSource = struct.input().asFileSource();
- String delimiter = fileSource.delimiter();
- if (Optional.ofNullable(delimiter).isPresent()) {
- elements = builder.build(fileSource.header(),
- row.mkString(delimiter).split(delimiter));
+ if (fileSource.format() == FileFormat.TEXT) {
+ if (row.isNullAt(0) || "".equals(row.getString(0))) {
+ return;
+ }
+ Line line = new TextLineParser(fileSource.delimiter())
+ .parse(fileSource.header(), row.getString(0));
+ elements = builder.build(line.names(), line.values());
} else {
- //elements = builder.build(row);
- String[] names = row.schema().fieldNames();
+ String[] names = fileSource.format() == FileFormat.CSV ?
+ fileSource.header() :
row.schema().fieldNames();
+ if (names.length != row.size()) {
Review Comment:
Minor: `header` is optional in `AbstractSource` (it defaults to null, and
the ordinary Loader reads it from the file). A CSV or TEXT mapping without an
explicit `header` therefore reaches `names.length` here, or `header.length` in
`TextLineParser.parse` at line 306, and fails on the executor with a
NullPointerException instead of a clear error. A mapping with `has_header:
true` is accepted and its physical header row is loaded as data.
docs/spark-java17.md already lists both as limitations. Please reject a null
header and `has_header=true` on the driver for CSV/TEXT sources before
submitting partitions, with a message that points to that doc.
##########
.github/scripts/verify_candidate_image_sdk.py:
##########
@@ -20,10 +20,14 @@
import argparse
import hashlib
import json
+import os
+import re
+from functools import lru_cache
+import urllib.request
from pathlib import Path
REPOSITORY = "apache/hugegraph"
-COMMIT = "d9abcd4317fb36128e7e4d209139ec7f3a28cdfc"
+COMMIT = "d9abcd"
Review Comment:
Important: Moving from 40-character SHAs to `d9abcd` means the lock no
longer pins anything. In `resolve.cjs`, when both `ref` and `expected-commit`
are the same six-character prefix, both values come from the same
`repos.getCommit` lookup of `d9abcd`, so the 'ref moved' check compares the
lookup with itself. Here, `source_commit()` resolves `d9abcd` through the
GitHub API again at verify time and compares the manifest to that answer, not
to a committed identity. The Dockerfiles and `checkout-server.sh` resolve the
same prefix the same way. A six-hex prefix has only 24 bits, and the commits
API also resolves objects that outsiders can add to apache/hugegraph: `gh api
-X GET repos/apache/hugegraph/commits/020b2d` resolves `020b2dfb...`, a commit
that is only on a PR ref (refs/pull). Anyone can open a PR with a commit that
starts with `d9abcd` (a quick brute force). Per docs/java17-migration.md the
prefix then becomes ambiguous and fails, which breaks every SDK-dependent CI
job and
both product image builds. The Docker builds also now need unauthenticated
GitHub API access inside `docker build`. Please keep the full 40-character SHA
in `expected-commit`, `COMMIT`, ci.yml and both Dockerfiles. Shorten it only in
log output.
--
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]
