Copilot commented on code in PR #2801:
URL: https://github.com/apache/sedona/pull/2801#discussion_r3007774597
##########
common/src/test/java/org/apache/sedona/common/FunctionsProj4PerformanceTest.java:
##########
@@ -202,11 +206,11 @@ public void testCacheEffectRemoteFetchEpsgCode() {
double warmAvgUs = (warmTotalMs * 1000) / WARM_ITERATIONS;
printResult("Remote Fetch EPSG", coldMs, warmAvgUs,
Proj4.getCacheSize());
- System.out.printf("Note: Cold time includes network fetch from
spatialreference.org%n");
+ log.info("Note: Cold time includes network fetch from
spatialreference.org");
assertNotNull(coldResult);
assertEquals(2154, coldResult.getSRID());
} catch (Exception e) {
- System.out.println("Skipped: Network fetch failed - " + e.getMessage());
+ log.info("Skipped: Network fetch failed - {}", e.getMessage());
// Don't fail the test if network is unavailable
Review Comment:
This test swallows network failures and only logs an INFO message. Since the
test log level is now WARN, the skip reason will usually be suppressed and the
test will appear to pass without actually exercising the remote-fetch path.
Prefer using JUnit `Assume` (mark the test as skipped with a message) or
logging the skip at WARN so the outcome is visible.
```suggestion
log.warn("Skipping testCacheEffectRemoteFetchEpsgCode: network fetch
failed - {}", e.getMessage());
// Mark the test as skipped if network is unavailable
assumeTrue("Network fetch failed: " + e.getMessage(), false);
```
##########
common/src/test/java/org/apache/sedona/common/FunctionsProj4PerformanceTest.java:
##########
@@ -337,12 +341,12 @@ public void testCacheEffectGridFileRemote() {
double warmAvgUs = (warmTotalMs * 1000) / WARM_ITERATIONS;
printResult("Grid File (remote)", coldMs, warmAvgUs,
Proj4.getCacheSize());
- System.out.printf("Grid cache entries: %10d%n",
NadgridRegistry.size());
- System.out.printf("Note: Cold time includes downloading grid file
(~15MB)%n");
+ log.info(String.format("Grid cache entries: %10d",
NadgridRegistry.size()));
+ log.info("Note: Cold time includes downloading grid file (~15MB)");
assertNotNull(coldResult);
assertTrue(NadgridRegistry.size() > 0);
} catch (Exception e) {
- System.out.println("Skipped: Remote grid download failed - " +
e.getMessage());
+ log.info("Skipped: Remote grid download failed - {}", e.getMessage());
// Don't fail the test if network is unavailable
Review Comment:
Same as the remote EPSG test: the remote grid download failure is swallowed
and only logged at INFO. With root logger set to WARN in tests, this will
usually be invisible and the test can pass without running the intended code
path. Consider using JUnit `Assume` to mark it skipped (with a message) or
logging the skip at WARN.
```suggestion
log.warn("Skipped: Remote grid download failed - {}", e.getMessage());
// Mark test as skipped if network or remote grid is unavailable
assumeTrue("Skipped: Remote grid download failed - " + e.getMessage(),
false);
```
##########
common/src/test/java/org/apache/sedona/common/FunctionsProj4PerformanceTest.java:
##########
@@ -70,19 +73,19 @@ private Point createTestPoint(double lon, double lat) {
}
private void printHeader(String title) {
- System.out.println();
- System.out.println("=".repeat(70));
- System.out.println(title);
- System.out.println("=".repeat(70));
+ log.info("");
+ log.info("=".repeat(70));
+ log.info(title);
+ log.info("=".repeat(70));
}
private void printResult(String label, double coldMs, double warmAvgUs, int
cacheEntries) {
double speedup = (coldMs * 1000) / warmAvgUs;
- System.out.printf("Cold (1 call): %10.2f ms%n", coldMs);
- System.out.printf("Warm (%d calls): %10.2f μs avg%n", WARM_ITERATIONS,
warmAvgUs);
- System.out.printf("Cache speedup: %10.0fx%n", speedup);
+ log.info(String.format("Cold (1 call): %10.2f ms", coldMs));
+ log.info(String.format("Warm (%d calls): %10.2f μs avg",
WARM_ITERATIONS, warmAvgUs));
+ log.info(String.format("Cache speedup: %10.0fx", speedup));
if (cacheEntries >= 0) {
- System.out.printf("Proj cache entries: %10d%n", cacheEntries);
+ log.info(String.format("Proj cache entries: %10d", cacheEntries));
}
Review Comment:
`printHeader`/`printResult` uses `String.format(...)` and other eager string
construction before calling `log.info(...)`. With the test log level set to
WARN, these INFO logs are typically disabled, but the formatting cost is still
paid. Consider switching to SLF4J parameterized logging (or guarding with
`log.isInfoEnabled()`) to avoid unnecessary work when INFO is off.
##########
spark/common/src/main/java/org/apache/sedona/core/formatMapper/shapefileParser/ShapefileRDD.java:
##########
@@ -75,7 +79,7 @@ public Geometry call(Tuple2<ShapeKey, PrimitiveShape>
primitiveTuple) {
private final VoidFunction<Geometry> PrintShape =
new VoidFunction<Geometry>() {
public void call(Geometry shape) throws Exception {
- System.out.println(shape.toText());
+ log.debug("{}", shape.toText());
Review Comment:
`log.debug("{}", shape.toText())` eagerly builds the WKT string even when
DEBUG is disabled, which can be expensive for large geometries. Prefer passing
the `Geometry` as the argument (so `toString`/formatting is only invoked when
enabled) or guard the `toText()` call behind `log.isDebugEnabled()`.
```suggestion
log.debug("{}", shape);
```
--
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]
