mihaibudiu commented on code in PR #4833:
URL: https://github.com/apache/calcite/pull/4833#discussion_r2925979337
##########
redis/src/main/java/org/apache/calcite/adapter/redis/RedisEnumerator.java:
##########
@@ -43,7 +41,7 @@ class RedisEnumerator implements Enumerator<Object[]> {
redisConfig.getDatabase(), redisConfig.getPassword());
try (Jedis jedis = redisManager.getResource()) {
- if (StringUtils.isNotEmpty(redisConfig.getPassword())) {
+ if (redisConfig.getPassword() != null &&
!redisConfig.getPassword().isEmpty()) {
Review Comment:
This calls getPassword twice, can you just save it to a variable?
##########
redis/src/main/java/org/apache/calcite/adapter/redis/RedisSchema.java:
##########
@@ -92,15 +90,15 @@ public RedisTableFieldInfo getTableFieldInfo(String
tableName) {
if (jsonCustomTable.name.equals(tableName)) {
Map<String, Object> map =
requireNonNull(jsonCustomTable.operand, OPERAND);
- if (ObjectUtils.isEmpty(map.get(DATA_FORMAT))) {
+ if (Util.isEmptyObject(map.get(DATA_FORMAT))) {
Review Comment:
Since this function is only used here, I would move it from Util to this
file and make it private.
##########
core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java:
##########
@@ -372,6 +370,20 @@ public interface Config extends RelRule.Config {
+ "this: JavaType(class java.lang.Integer) -> JavaType(void) NOT
NULL\n"));
}
+ private static Throwable rootCause(Throwable t) {
+ Throwable cur = t;
+ // Safety against loops in cause chain.
+ final java.util.IdentityHashMap<Throwable, Boolean> seen =
Review Comment:
Why a Map and not a Set?
##########
file/src/main/java/org/apache/calcite/adapter/file/CsvEnumerator.java:
##########
@@ -76,20 +75,35 @@ public class CsvEnumerator<E> implements Enumerator<E> {
private final RowConverter<E> rowConverter;
private @Nullable E current;
- private static final FastDateFormat TIME_FORMAT_DATE;
- private static final FastDateFormat TIME_FORMAT_TIME;
- private static final FastDateFormat TIME_FORMAT_TIMESTAMP;
+ private static final TimeZone GMT = TimeZone.getTimeZone("GMT");
Review Comment:
These I haven't checked whether they are equivalent with the originals...
##########
geode/src/main/java/org/apache/calcite/adapter/geode/util/GeodeUtils.java:
##########
@@ -117,6 +116,10 @@ public static synchronized void closeClientCache() {
REGION_MAP.clear();
}
+ private static boolean equalsIgnoreCase(@Nullable String a, @Nullable String
b) {
Review Comment:
this could be in Util, but it's not a must
##########
plus/src/test/java/org/apache/calcite/adapter/os/OsAdapterTest.java:
##########
@@ -114,7 +108,7 @@ private static boolean checkProcessExists(String command) {
}
@Test void testDu() {
- assumeFalse(isWindows(), "Skip: the 'du' table does not work on Windows");
+ assumeFalse(Util.isWindows(), "Skip: the 'du' table does not work on
Windows");
Review Comment:
These are not 100% equivalent, but hopefully they work the same in practice.
--
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]
