From 6c31a4a4ad1a9a1069d476fe024bc3473f4eae94 Mon Sep 17 00:00:00 2001 From: cgivre Date: Wed, 30 Apr 2025 11:16:16 -0400 Subject: [PATCH 01/12] DRILL-8523: Remove Support for Java 8 --- .github/workflows/ci.yml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 243c0091856..bc513ac1073 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,11 +34,8 @@ jobs: strategy: matrix: # Java versions to run unit tests - java: [ '8', '11', '17' ] + java: [ '11', '17', '21' ] profile: ['default-hadoop'] - include: - - java: '8' - profile: 'hadoop-2' fail-fast: false steps: - name: Checkout From 24b25eb946a6dee42ca2995da57e44bf518e51fd Mon Sep 17 00:00:00 2001 From: cgivre Date: Wed, 30 Apr 2025 11:45:08 -0400 Subject: [PATCH 02/12] Various fixes --- pom.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pom.xml b/pom.xml index 5e96f05fe28..44e286b0aea 100644 --- a/pom.xml +++ b/pom.xml @@ -215,7 +215,7 @@ org.owasp dependency-check-maven - 6.4.1 + 12.1.1 @@ -505,7 +505,7 @@ [${maven.version.min},4) - [1.8,18) + [1.8,22) From 603031161b6a8c35b286790c56fc9d777c22e7f6 Mon Sep 17 00:00:00 2001 From: cgivre Date: Wed, 30 Apr 2025 13:33:11 -0400 Subject: [PATCH 03/12] Update mockito --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 44e286b0aea..d3674b8bcda 100644 --- a/pom.xml +++ b/pom.xml @@ -119,7 +119,7 @@ 3.8.4 3072 4.2.19 - 3.11.2 + 5.17.0 4.11.1 0.6.6 From 60578d001e31ba0baa471b5e4dacc8985345f213 Mon Sep 17 00:00:00 2001 From: cgivre Date: Wed, 30 Apr 2025 13:57:59 -0400 Subject: [PATCH 04/12] Fix mockito updates --- .../java/org/apache/drill/PlanningBase.java | 33 +++++++++---------- .../drill/exec/rpc/data/TestBitRpc.java | 2 +- .../vector/accessor/GenericAccessorTest.java | 2 +- .../fragment/FragmentStatusReporterTest.java | 8 ++--- pom.xml | 3 +- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java b/exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java index 1106ea18f4b..1cdc07afd5c 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java +++ b/exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java @@ -17,29 +17,26 @@ */ package org.apache.drill; -import java.io.IOException; -import java.net.URL; -import java.nio.charset.StandardCharsets; - -import org.apache.calcite.jdbc.DynamicSchema; -import org.apache.drill.exec.alias.AliasRegistryProvider; -import org.apache.drill.exec.ops.ViewExpansionContext; +import com.codahale.metrics.MetricRegistry; import com.google.common.base.Function; +import com.google.common.collect.ImmutableList; +import com.google.common.io.Resources; import io.netty.buffer.DrillBuf; +import org.apache.calcite.jdbc.DynamicSchema; import org.apache.calcite.schema.SchemaPlus; import org.apache.drill.common.config.DrillConfig; import org.apache.drill.common.config.LogicalPlanPersistence; import org.apache.drill.common.scanner.ClassPathScanner; import org.apache.drill.common.scanner.persistence.ScanResult; import org.apache.drill.common.types.TypeProtos; -import org.apache.drill.exec.expr.holders.ValueHolder; -import org.apache.drill.exec.vector.ValueHolderHelper; -import org.apache.drill.test.TestTools; import org.apache.drill.exec.ExecTest; +import org.apache.drill.exec.alias.AliasRegistryProvider; import org.apache.drill.exec.expr.fn.FunctionImplementationRegistry; +import org.apache.drill.exec.expr.holders.ValueHolder; import org.apache.drill.exec.memory.BufferAllocator; import org.apache.drill.exec.memory.RootAllocatorFactory; import org.apache.drill.exec.ops.QueryContext; +import org.apache.drill.exec.ops.ViewExpansionContext; import org.apache.drill.exec.physical.PhysicalPlan; import org.apache.drill.exec.planner.physical.PlannerSettings; import org.apache.drill.exec.planner.sql.DrillOperatorTable; @@ -55,16 +52,18 @@ import org.apache.drill.exec.store.StoragePluginRegistryImpl; import org.apache.drill.exec.store.sys.store.provider.LocalPersistentStoreProvider; import org.apache.drill.exec.testing.ExecutionControls; +import org.apache.drill.exec.vector.ValueHolderHelper; +import org.apache.drill.test.TestTools; import org.junit.Rule; import org.junit.rules.TestRule; +import org.mockito.ArgumentMatchers; -import com.codahale.metrics.MetricRegistry; -import com.google.common.collect.ImmutableList; -import com.google.common.io.Resources; -import org.mockito.Matchers; +import java.io.IOException; +import java.net.URL; +import java.nio.charset.StandardCharsets; import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Matchers.eq; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -131,11 +130,11 @@ protected void testSqlPlan(String sqlCommands) throws Exception { when(context.getManagedBuffer()).thenReturn(allocator.buffer(4)); when(context.getConstantValueHolder(eq("0.03"), eq(TypeProtos.MinorType.VARDECIMAL), - Matchers.>any())) + ArgumentMatchers.>any())) .thenReturn(ValueHolderHelper.getVarDecimalHolder(allocator.buffer(4), "0.03")); when(context.getConstantValueHolder(eq("0.01"), eq(TypeProtos.MinorType.VARDECIMAL), - Matchers.>any())) + ArgumentMatchers.>any())) .thenReturn(ValueHolderHelper.getVarDecimalHolder(allocator.buffer(4), "0.01")); when(context.getOption(anyString())).thenCallRealMethod(); when(context.getViewExpansionContext()).thenReturn(viewExpansionContext); diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/rpc/data/TestBitRpc.java b/exec/java-exec/src/test/java/org/apache/drill/exec/rpc/data/TestBitRpc.java index c162a7625fb..439dedd8db9 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/rpc/data/TestBitRpc.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/rpc/data/TestBitRpc.java @@ -56,7 +56,7 @@ import java.util.concurrent.atomic.AtomicLong; import static org.junit.Assert.assertTrue; -import static org.mockito.Matchers.any; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/vector/accessor/GenericAccessorTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/vector/accessor/GenericAccessorTest.java index 57a01911cdf..41afede8c18 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/vector/accessor/GenericAccessorTest.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/vector/accessor/GenericAccessorTest.java @@ -31,7 +31,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import static org.mockito.Matchers.anyInt; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/work/fragment/FragmentStatusReporterTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/work/fragment/FragmentStatusReporterTest.java index b4bc9cc76e3..5eb7f80680d 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/work/fragment/FragmentStatusReporterTest.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/work/fragment/FragmentStatusReporterTest.java @@ -34,11 +34,11 @@ import static org.apache.drill.exec.proto.UserBitShared.FragmentState.FAILED; import static org.apache.drill.exec.proto.UserBitShared.FragmentState.RUNNING; import static org.junit.Assert.fail; -import static org.mockito.Matchers.any; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyZeroInteractions; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; public class FragmentStatusReporterTest extends BaseTest { @@ -93,14 +93,14 @@ public void testFail() throws Exception { @Test public void testClose() throws Exception { statusReporter.close(); - verifyZeroInteractions(foremanTunnel); + verifyNoInteractions(foremanTunnel); } @Test public void testCloseClosed() throws Exception { statusReporter.close(); statusReporter.close(); - verifyZeroInteractions(foremanTunnel); + verifyNoInteractions(foremanTunnel); } @Test diff --git a/pom.xml b/pom.xml index d3674b8bcda..834104c06ce 100644 --- a/pom.xml +++ b/pom.xml @@ -120,6 +120,7 @@ 3072 4.2.19 5.17.0 + 5.2.0 4.11.1 0.6.6 @@ -918,7 +919,7 @@ org.mockito mockito-inline - ${mockito.version} + ${mockito_inline.version} test From 3a9909713fef97fc9372b96c83cca4c995ec991a Mon Sep 17 00:00:00 2001 From: cgivre Date: Wed, 30 Apr 2025 14:31:42 -0400 Subject: [PATCH 05/12] More mockito fixes --- pom.xml | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/pom.xml b/pom.xml index 834104c06ce..80c2fcf94d7 100644 --- a/pom.xml +++ b/pom.xml @@ -113,8 +113,8 @@ 2.23.1 1.3.15 - 1.8 - 1.8 + 11 + 11 3.6.3 3.8.4 3072 @@ -163,27 +163,27 @@ user-subscribe@drill.apache.org user-unsubscribe@drill.apache.org user@drill.apache.org - http://mail-archives.apache.org/mod_mbox/drill-user/ + https://mail-archives.apache.org/mod_mbox/drill-user/ Developer List dev-subscribe@drill.apache.org dev-unsubscribe@drill.apache.org dev@drill.apache.org - http://mail-archives.apache.org/mod_mbox/drill-dev/ + https://mail-archives.apache.org/mod_mbox/drill-dev/ Commits List commits-subscribe@drill.apache.org commits-unsubscribe@drill.apache.org commits@drill.apache.org - http://mail-archives.apache.org/mod_mbox/drill-commits/ + https://mail-archives.apache.org/mod_mbox/drill-commits/ Issues List issues-subscribe@drill.apache.org issues-unsubscribe@drill.apache.org - http://mail-archives.apache.org/mod_mbox/drill-issues/ + https://mail-archives.apache.org/mod_mbox/drill-issues/ @@ -914,6 +914,12 @@ org.mockito mockito-core ${mockito.version} + + + org.mockito + mockito-core + + test From 64335a5f8e196c3ba434c94ed0a0bd66d8ebb989 Mon Sep 17 00:00:00 2001 From: cgivre Date: Wed, 30 Apr 2025 14:40:32 -0400 Subject: [PATCH 06/12] Hopefully the last pom fix --- pom.xml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pom.xml b/pom.xml index 80c2fcf94d7..835ddd4c385 100644 --- a/pom.xml +++ b/pom.xml @@ -113,8 +113,7 @@ 2.23.1 1.3.15 - 11 - 11 + 11 3.6.3 3.8.4 3072 From c89e13df0ccdad5efbec16318eef0b5247d52fd1 Mon Sep 17 00:00:00 2001 From: cgivre Date: Wed, 30 Apr 2025 14:44:27 -0400 Subject: [PATCH 07/12] Sigh --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 835ddd4c385..74e39af4067 100644 --- a/pom.xml +++ b/pom.xml @@ -2929,7 +2929,7 @@ [9,) - 8 + 11 --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED From b6b785ee506d53e28370e5f1f6d9036a722640ef Mon Sep 17 00:00:00 2001 From: cgivre Date: Wed, 30 Apr 2025 14:50:14 -0400 Subject: [PATCH 08/12] More version updates --- .github/workflows/ci.yml | 2 +- pom.xml | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bc513ac1073..c148858e70e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -79,7 +79,7 @@ jobs: uses: actions/setup-java@v4 with: distribution: 'temurin' - java-version: '8' + java-version: '11' cache: 'maven' # Caches built protobuf library - name: Cache protobufs diff --git a/pom.xml b/pom.xml index 74e39af4067..23279a1344e 100644 --- a/pom.xml +++ b/pom.xml @@ -114,6 +114,8 @@ 1.3.15 11 + 11 + 11 3.6.3 3.8.4 3072 @@ -2929,6 +2931,8 @@ [9,) + 11 + 11 11 --add-opens java.base/java.lang=ALL-UNNAMED From e039984a983afa872c55767aa13cb71bf28164f9 Mon Sep 17 00:00:00 2001 From: cgivre Date: Thu, 1 May 2025 09:58:55 -0400 Subject: [PATCH 09/12] WIP --- .../impl/TestNestedDateTimeTimestamp.java | 27 ++++++++++++------- pom.xml | 5 ++-- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestNestedDateTimeTimestamp.java b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestNestedDateTimeTimestamp.java index 05697aa60ae..27ab6d16b0a 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestNestedDateTimeTimestamp.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestNestedDateTimeTimestamp.java @@ -17,6 +17,16 @@ */ package org.apache.drill.exec.physical.impl; +import org.apache.drill.categories.FlakyTest; +import org.apache.drill.exec.expr.fn.impl.DateUtility; +import org.apache.drill.exec.rpc.user.QueryDataBatch; +import org.apache.drill.test.BaseTestQuery; +import org.apache.drill.test.TestBuilder; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; + import java.time.Instant; import java.time.LocalDate; import java.time.LocalDateTime; @@ -27,17 +37,9 @@ import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.TimeZone; import java.util.TreeMap; -import org.apache.drill.categories.FlakyTest; -import org.apache.drill.exec.expr.fn.impl.DateUtility; -import org.apache.drill.exec.rpc.user.QueryDataBatch; -import org.apache.drill.test.BaseTestQuery; -import org.apache.drill.test.TestBuilder; -import org.junit.Assert; -import org.junit.Test; -import org.junit.experimental.categories.Category; - /** * For DRILL-6242, output for Date, Time, Timestamp should use different classes */ @@ -46,6 +48,13 @@ public class TestNestedDateTimeTimestamp extends BaseTestQuery { private static final String DATAFILE = "cp.`datetime.parquet`"; private static final Map expectedRecord = new TreeMap(); + @BeforeClass + public static void setUpTimeZone() { + // Certain tests in this class fail if your machine is not set to UTC. This method + // sets the default timezone to UTC when these tests are run. + TimeZone.setDefault(TimeZone.getTimeZone("UTC")); + } + static { /** * Data in the parquet file represents this equivalent JSON, but with typed diff --git a/pom.xml b/pom.xml index 23279a1344e..a781b6b1ce9 100644 --- a/pom.xml +++ b/pom.xml @@ -94,7 +94,7 @@ 5.10.0 0.12.1 2.18.3 - 3.1.11 + 3.1.12 3.29.2-GA 3.0.0 2.0.1.Final @@ -118,7 +118,7 @@ 11 3.6.3 3.8.4 - 3072 + 4096 4.2.19 5.17.0 5.2.0 @@ -629,6 +629,7 @@ -XX:MaxDirectMemorySize=${directMemoryMb}M -Djava.net.preferIPv4Stack=true -Djava.awt.headless=true + -Duser.timezone=UTC -ea ${junit.args} -Djdk.attach.allowAttachSelf=true From c28a7b8b47795186355b5b3720623b1dfa4ff186 Mon Sep 17 00:00:00 2001 From: cgivre Date: Thu, 1 May 2025 12:19:20 -0400 Subject: [PATCH 10/12] WIP --- .../exec/physical/impl/writer/TestParquetWriter.java | 12 ++++++++---- exec/jdbc-all/pom.xml | 2 +- pom.xml | 8 +++++++- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java index 076bb2b45b6..8a0e3ad3ed4 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java @@ -17,6 +17,8 @@ */ package org.apache.drill.exec.physical.impl.writer; +import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableList; import org.apache.calcite.util.Pair; import org.apache.commons.io.FileUtils; import org.apache.drill.categories.ParquetTest; @@ -30,8 +32,6 @@ import org.apache.drill.exec.store.dfs.FileSystemConfig; import org.apache.drill.exec.store.parquet.ParquetFormatConfig; import org.apache.drill.exec.util.JsonStringArrayList; -import com.google.common.base.Joiner; -import com.google.common.collect.ImmutableList; import org.apache.drill.test.ClusterFixture; import org.apache.drill.test.ClusterTest; import org.apache.drill.test.TestBuilder; @@ -41,16 +41,18 @@ import org.apache.hadoop.fs.Path; import org.apache.parquet.hadoop.ParquetFileReader; import org.apache.parquet.hadoop.metadata.ParquetMetadata; -import org.apache.parquet.schema.MessageType; import org.apache.parquet.schema.LogicalTypeAnnotation; +import org.apache.parquet.schema.MessageType; import org.apache.parquet.schema.PrimitiveType; import org.joda.time.Period; - import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Ignore; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.EnabledIfSystemProperty; +import org.junit.jupiter.api.condition.OS; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -1002,6 +1004,8 @@ public void testTPCHReadWriteGzip() throws Exception { // Only attempt this test on Linux / amd64 because com.rdblue.brotli-codec // only bundles natives for Mac and Linux on AMD64. See PARQUET-1975. @Test + @EnabledIfSystemProperty(named = "os.arch", matches = "(amd64|x86_64)") + @DisabledOnOs({ OS.WINDOWS, OS.MAC }) public void testTPCHReadWriteBrotli() throws Exception { try { client.alterSession(ExecConstants.PARQUET_WRITER_COMPRESSION_TYPE, "brotli"); diff --git a/exec/jdbc-all/pom.xml b/exec/jdbc-all/pom.xml index 1b7fef416c5..6a2159c2213 100644 --- a/exec/jdbc-all/pom.xml +++ b/exec/jdbc-all/pom.xml @@ -33,7 +33,7 @@ "package.namespace.prefix" equals to "oadd.". It can be overridden if necessary within any profile --> oadd. - 55000000 + 56000000 diff --git a/pom.xml b/pom.xml index a781b6b1ce9..4cffc55d72c 100644 --- a/pom.xml +++ b/pom.xml @@ -354,7 +354,6 @@ **/*.java - UTF-8 true true true @@ -886,6 +885,7 @@ ${junit.platform.version} test + org.junit.platform junit-platform-suite-engine @@ -929,6 +929,12 @@ mockito-inline ${mockito_inline.version} test + + + mockito-core + org.mockito + + de.huxhorn.lilith From 4f3268ff66a7d400e622a30db8db893f05ebb092 Mon Sep 17 00:00:00 2001 From: cgivre Date: Mon, 5 May 2025 10:50:37 -0400 Subject: [PATCH 11/12] Fixed ClassTransformer to avoid duplicate class insertion --- .../exec/compile/ClassCompilerSelector.java | 12 +-- .../drill/exec/compile/ClassTransformer.java | 89 +++++++++---------- .../drill/exec/compile/QueryClassLoader.java | 21 ++--- .../exec/compile/TestClassTransformation.java | 16 ++-- 4 files changed, 68 insertions(+), 70 deletions(-) diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/ClassCompilerSelector.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/ClassCompilerSelector.java index f35942eac8b..e5c40139bf0 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/ClassCompilerSelector.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/ClassCompilerSelector.java @@ -17,10 +17,6 @@ */ package org.apache.drill.exec.compile; -import java.io.IOException; -import java.util.Arrays; -import java.util.Map; - import org.apache.drill.common.config.DrillConfig; import org.apache.drill.common.exceptions.UserException; import org.apache.drill.exec.compile.ClassTransformer.ClassNames; @@ -34,6 +30,12 @@ import org.apache.drill.exec.server.options.TypeValidators.LongValidator; import org.apache.drill.exec.server.options.TypeValidators.StringValidator; import org.codehaus.commons.compiler.CompileException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Map; /** * Selects between the two supported Java compilers: Janino and @@ -65,7 +67,7 @@ */ public class ClassCompilerSelector { - private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(ClassCompilerSelector.class); + private static final Logger logger = LoggerFactory.getLogger(ClassCompilerSelector.class); public enum CompilerPolicy { DEFAULT, JDK, JANINO; diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/ClassTransformer.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/ClassTransformer.java index e777a883d1f..83e401f2485 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/ClassTransformer.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/ClassTransformer.java @@ -17,11 +17,11 @@ */ package org.apache.drill.exec.compile; -import java.io.IOException; -import java.util.LinkedList; -import java.util.Map; -import java.util.Set; - +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import org.apache.commons.lang3.tuple.Pair; import org.apache.drill.common.config.DrillConfig; import org.apache.drill.common.util.DrillFileUtils; @@ -35,11 +35,10 @@ import org.objectweb.asm.ClassReader; import org.objectweb.asm.tree.ClassNode; -import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Preconditions; -import com.google.common.collect.Lists; -import com.google.common.collect.Maps; -import com.google.common.collect.Sets; +import java.io.IOException; +import java.util.LinkedList; +import java.util.Map; +import java.util.Set; /** * Compiles generated code, merges the resulting class with the @@ -52,7 +51,7 @@ public class ClassTransformer { private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(ClassTransformer.class); - private static final int MAX_SCALAR_REPLACE_CODE_SIZE = 2*1024*1024; // 2meg + private static final int MAX_SCALAR_REPLACE_CODE_SIZE = 2 * 1024 * 1024; // 2meg private final ByteCodeLoader byteCodeLoader = new ByteCodeLoader(); private final DrillConfig config; @@ -72,7 +71,7 @@ public enum ScalarReplacementOption { * @throws IllegalArgumentException if the string doesn't match any of the enum values */ public static ScalarReplacementOption fromString(final String s) { - switch(s) { + switch (s) { case "off": return OFF; case "try": @@ -228,8 +227,11 @@ public Class getImplementationClass( final TemplateClassDefinition templateDefinition, final String entireClass, final String materializedClassName) throws ClassTransformationException { - // unfortunately, this hasn't been set up at construction time, so we have to do it here - final ScalarReplacementOption scalarReplacementOption = ScalarReplacementOption.fromString(optionManager.getOption(ExecConstants.SCALAR_REPLACEMENT_VALIDATOR)); + final ScalarReplacementOption scalarReplacementOption = + ScalarReplacementOption.fromString(optionManager.getOption(ExecConstants.SCALAR_REPLACEMENT_VALIDATOR)); + + // Track injected class names to avoid duplicates + Set injectedClassNames = new java.util.HashSet<>(); try { final long t1 = System.nanoTime(); @@ -251,7 +253,7 @@ public Class getImplementationClass( final Set namesCompleted = Sets.newHashSet(); names.add(set); - while ( !names.isEmpty() ) { + while (!names.isEmpty()) { final ClassSet nextSet = names.removeFirst(); if (namesCompleted.contains(nextSet)) { continue; @@ -259,45 +261,24 @@ public Class getImplementationClass( final ClassNames nextPrecompiled = nextSet.precompiled; final byte[] precompiledBytes = byteCodeLoader.getClassByteCodeFromPath(nextPrecompiled.clazz); final ClassNames nextGenerated = nextSet.generated; - // keeps only classes that have not be merged + // keeps only classes that have not been merged Pair classNodePair = classesToMerge.remove(nextGenerated.slash); - final ClassNode generatedNode; - if (classNodePair != null) { - generatedNode = classNodePair.getValue(); - } else { - generatedNode = null; - } + final ClassNode generatedNode = (classNodePair != null) ? classNodePair.getValue() : null; - /* - * TODO - * We're having a problem with some cases of scalar replacement, but we want to get - * the code in so it doesn't rot anymore. - * - * Here, we use the specified replacement option. The loop will allow us to retry if - * we're using TRY. - */ MergedClassResult result = null; - boolean scalarReplace = scalarReplacementOption != ScalarReplacementOption.OFF && entireClass.length() < MAX_SCALAR_REPLACE_CODE_SIZE; - while(true) { + boolean scalarReplace = scalarReplacementOption != ScalarReplacementOption.OFF + && entireClass.length() < MAX_SCALAR_REPLACE_CODE_SIZE; + while (true) { try { result = MergeAdapter.getMergedClass(nextSet, precompiledBytes, generatedNode, scalarReplace); break; - } catch(RuntimeException e) { - // if we had a problem without using scalar replacement, then rethrow + } catch (RuntimeException e) { if (!scalarReplace) { throw e; } - - // if we did try to use scalar replacement, decide if we need to retry or not if (scalarReplacementOption == ScalarReplacementOption.ON) { - // option is forced on, so this is a hard error throw e; } - - /* - * We tried to use scalar replacement, with the option to fall back to not using it. - * Log this failure before trying again without scalar replacement. - */ logger.info("scalar replacement failure (retrying)\n", e); scalarReplace = false; } @@ -307,26 +288,38 @@ public Class getImplementationClass( s = s.replace(DrillFileUtils.SEPARATOR_CHAR, '.'); names.add(nextSet.getChild(s)); } - classLoader.injectByteCode(nextGenerated.dot, result.bytes); + + // Only inject bytecode if not already injected + if (!injectedClassNames.contains(nextGenerated.dot)) { + classLoader.injectByteCode(nextGenerated.dot, result.bytes); + injectedClassNames.add(nextGenerated.dot); + } + namesCompleted.add(nextSet); } // adds byte code of the classes that have not been merged to make them accessible for outer class for (Map.Entry> clazz : classesToMerge.entrySet()) { - classLoader.injectByteCode(clazz.getKey().replace(DrillFileUtils.SEPARATOR_CHAR, '.'), clazz.getValue().getKey()); + String classNameDot = clazz.getKey().replace(DrillFileUtils.SEPARATOR_CHAR, '.'); + if (!injectedClassNames.contains(classNameDot)) { + classLoader.injectByteCode(classNameDot, clazz.getValue().getKey()); + injectedClassNames.add(classNameDot); + } } + Class c = classLoader.findClass(set.generated.dot); if (templateDefinition.getExternalInterface().isAssignableFrom(c)) { logger.debug("Compiled and merged {}: bytecode size = {}, time = {} ms.", - c.getSimpleName(), - DrillStringUtils.readable(totalBytecodeSize), - (System.nanoTime() - t1 + 500_000) / 1_000_000); + c.getSimpleName(), + DrillStringUtils.readable(totalBytecodeSize), + (System.nanoTime() - t1 + 500_000) / 1_000_000); return c; } throw new ClassTransformationException("The requested class did not implement the expected interface."); } catch (CompileException | IOException | ClassNotFoundException e) { - throw new ClassTransformationException(String.format("Failure generating transformation classes for value: \n %s", entireClass), e); + throw new ClassTransformationException( + String.format("Failure generating transformation classes for value: \n %s", entireClass), e); } } } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java index a9858b28472..825d6e95638 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java @@ -17,32 +17,33 @@ */ package org.apache.drill.exec.compile; -import java.io.IOException; -import java.net.URL; -import java.net.URLClassLoader; -import java.util.concurrent.ConcurrentMap; -import java.util.concurrent.atomic.AtomicLong; - +import com.google.common.collect.MapMaker; import org.apache.drill.common.config.DrillConfig; import org.apache.drill.exec.compile.ClassTransformer.ClassNames; import org.apache.drill.exec.exception.ClassTransformationException; import org.apache.drill.exec.server.options.OptionSet; import org.codehaus.commons.compiler.CompileException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; -import com.google.common.collect.MapMaker; +import java.io.IOException; +import java.net.URL; +import java.net.URLClassLoader; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.atomic.AtomicLong; /** * Per-compilation unit class loader that holds both caching and compilation * steps. */ public class QueryClassLoader extends URLClassLoader { - static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(QueryClassLoader.class); + static final Logger logger = LoggerFactory.getLogger(QueryClassLoader.class); - private ClassCompilerSelector compilerSelector; + private final ClassCompilerSelector compilerSelector; private AtomicLong index = new AtomicLong(0); - private ConcurrentMap customClasses = new MapMaker().concurrencyLevel(4).makeMap(); + private final ConcurrentMap customClasses = new MapMaker().concurrencyLevel(4).makeMap(); public QueryClassLoader(DrillConfig config, OptionSet sessionOptions) { super(new URL[0], Thread.currentThread().getContextClassLoader()); diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/compile/TestClassTransformation.java b/exec/java-exec/src/test/java/org/apache/drill/exec/compile/TestClassTransformation.java index de12fd74505..52d1951648b 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/compile/TestClassTransformation.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/compile/TestClassTransformation.java @@ -17,15 +17,10 @@ */ package org.apache.drill.exec.compile; -import java.io.IOException; -import java.util.Arrays; -import java.util.List; - import org.apache.drill.common.util.DrillFileUtils; import org.apache.drill.exec.ExecConstants; -import org.apache.drill.exec.compile.bytecode.ValueHolderReplacementVisitor; -import org.apache.drill.test.BaseTestQuery; import org.apache.drill.exec.compile.ClassTransformer.ClassSet; +import org.apache.drill.exec.compile.bytecode.ValueHolderReplacementVisitor; import org.apache.drill.exec.compile.sig.GeneratorMapping; import org.apache.drill.exec.compile.sig.MappingSet; import org.apache.drill.exec.exception.ClassTransformationException; @@ -33,6 +28,7 @@ import org.apache.drill.exec.expr.CodeGenerator; import org.apache.drill.exec.rpc.user.UserSession; import org.apache.drill.exec.server.options.SessionOptionManager; +import org.apache.drill.test.BaseTestQuery; import org.codehaus.commons.compiler.CompileException; import org.junit.Assert; import org.junit.BeforeClass; @@ -40,9 +36,15 @@ import org.objectweb.asm.ClassReader; import org.objectweb.asm.ClassWriter; import org.objectweb.asm.tree.ClassNode; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.Arrays; +import java.util.List; public class TestClassTransformation extends BaseTestQuery { - private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TestClassTransformation.class); + private static final Logger logger = LoggerFactory.getLogger(TestClassTransformation.class); private static final int ITERATION_COUNT = Integer.parseInt(System.getProperty("TestClassTransformation.iteration", "1")); From 9882237b7e6a858e03116b5d51244052bc184842 Mon Sep 17 00:00:00 2001 From: cgivre Date: Wed, 7 May 2025 10:08:29 -0400 Subject: [PATCH 12/12] Removed timezone hack from unit test --- .../impl/TestNestedDateTimeTimestamp.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestNestedDateTimeTimestamp.java b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestNestedDateTimeTimestamp.java index 27ab6d16b0a..a47db151de7 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestNestedDateTimeTimestamp.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestNestedDateTimeTimestamp.java @@ -23,7 +23,6 @@ import org.apache.drill.test.BaseTestQuery; import org.apache.drill.test.TestBuilder; import org.junit.Assert; -import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -37,24 +36,22 @@ import java.util.Arrays; import java.util.List; import java.util.Map; -import java.util.TimeZone; import java.util.TreeMap; /** * For DRILL-6242, output for Date, Time, Timestamp should use different classes + * + * Note that Drill treats all timestanps as naive (without timezone information). When running tests locally, + * these tests may fail if the local timezone is not UTC. To run tests on a machine with a non-UTC timezone, + * you should run the tests with the following command: + * + * mvn test -Duser.timezone=UTC */ @Category(FlakyTest.class) public class TestNestedDateTimeTimestamp extends BaseTestQuery { private static final String DATAFILE = "cp.`datetime.parquet`"; private static final Map expectedRecord = new TreeMap(); - @BeforeClass - public static void setUpTimeZone() { - // Certain tests in this class fail if your machine is not set to UTC. This method - // sets the default timezone to UTC when these tests are run. - TimeZone.setDefault(TimeZone.getTimeZone("UTC")); - } - static { /** * Data in the parquet file represents this equivalent JSON, but with typed