[CALCITE-915] Tests should unset ThreadLocal values on exit
Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/c104c75b Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/c104c75b Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/c104c75b Branch: refs/heads/master Commit: c104c75bdf6d663070cf16b5ab0f94f8c269c9b6 Parents: 7837e64 Author: Julian Hyde <[email protected]> Authored: Thu Oct 8 16:23:20 2015 -0700 Committer: Julian Hyde <[email protected]> Committed: Sun Jan 10 00:51:24 2016 -0800 ---------------------------------------------------------------------- .../org/apache/calcite/prepare/Prepare.java | 15 +- .../org/apache/calcite/util/TryThreadLocal.java | 71 ++++++++ .../java/org/apache/calcite/test/JdbcTest.java | 180 +++++++++---------- .../calcite/test/MaterializationTest.java | 21 +-- .../java/org/apache/calcite/util/UtilTest.java | 37 ++++ 5 files changed, 212 insertions(+), 112 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/c104c75b/core/src/main/java/org/apache/calcite/prepare/Prepare.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/prepare/Prepare.java b/core/src/main/java/org/apache/calcite/prepare/Prepare.java index afb722f..3530e93 100644 --- a/core/src/main/java/org/apache/calcite/prepare/Prepare.java +++ b/core/src/main/java/org/apache/calcite/prepare/Prepare.java @@ -52,6 +52,7 @@ import org.apache.calcite.tools.Program; import org.apache.calcite.tools.Programs; import org.apache.calcite.util.Holder; import org.apache.calcite.util.Pair; +import org.apache.calcite.util.TryThreadLocal; import org.apache.calcite.util.trace.CalciteTimingTracer; import org.apache.calcite.util.trace.CalciteTrace; @@ -83,12 +84,14 @@ public abstract class Prepare { protected RelDataType parameterRowType; // temporary. for testing. - public static final ThreadLocal<Boolean> THREAD_TRIM = - new ThreadLocal<Boolean>() { - @Override protected Boolean initialValue() { - return false; - } - }; + public static final TryThreadLocal<Boolean> THREAD_TRIM = + TryThreadLocal.of(false); + + /** Temporary, while CALCITE-816 is under development. + * + * @see org.apache.calcite.util.Util#deprecated(Object, boolean) */ + public static final TryThreadLocal<Boolean> THREAD_EXPAND = + TryThreadLocal.of(false); public Prepare(CalcitePrepare.Context context, CatalogReader catalogReader, Convention resultConvention) { http://git-wip-us.apache.org/repos/asf/calcite/blob/c104c75b/core/src/main/java/org/apache/calcite/util/TryThreadLocal.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/util/TryThreadLocal.java b/core/src/main/java/org/apache/calcite/util/TryThreadLocal.java new file mode 100644 index 0000000..b278174 --- /dev/null +++ b/core/src/main/java/org/apache/calcite/util/TryThreadLocal.java @@ -0,0 +1,71 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.calcite.util; + +/** + * Thread-local variable that returns a handle that can be closed. + * + * @param <T> Value type + */ +public class TryThreadLocal<T> extends ThreadLocal<T> { + private final T initialValue; + + /** Creates a TryThreadLocal. + * + * @param initialValue Initial value + */ + public static <T> TryThreadLocal<T> of(T initialValue) { + return new TryThreadLocal<>(initialValue); + } + + private TryThreadLocal(T initialValue) { + this.initialValue = initialValue; + } + + // It is important that this method is final. + // This ensures that the sub-class does not choose a different initial + // value. Then the close logic can detect whether the previous value was + // equal to the initial value. + @Override protected final T initialValue() { + return initialValue; + } + + /** Assigns the value as {@code value} for the current thread. + * Returns a {@link Memo} which, when closed, will assign the value + * back to the previous value. */ + public Memo push(T value) { + final T previous = get(); + set(value); + return new Memo() { + public void close() { + if (previous == initialValue) { + remove(); + } else { + set(previous); + } + } + }; + } + + /** Remembers to set the value back. */ + public interface Memo extends AutoCloseable { + /** Sets the value back; never throws. */ + @Override void close(); + } +} + +// End TryThreadLocal.java http://git-wip-us.apache.org/repos/asf/calcite/blob/c104c75b/core/src/test/java/org/apache/calcite/test/JdbcTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/test/JdbcTest.java b/core/src/test/java/org/apache/calcite/test/JdbcTest.java index b64cba4..243f903 100644 --- a/core/src/test/java/org/apache/calcite/test/JdbcTest.java +++ b/core/src/test/java/org/apache/calcite/test/JdbcTest.java @@ -91,6 +91,7 @@ import org.apache.calcite.util.Bug; import org.apache.calcite.util.JsonBuilder; import org.apache.calcite.util.Pair; import org.apache.calcite.util.Smalls; +import org.apache.calcite.util.TryThreadLocal; import org.apache.calcite.util.Util; import com.google.common.base.Function; @@ -247,8 +248,8 @@ public class JdbcTest { @Test public void testModelWithModifiableView() throws Exception { final List<Employee> employees = new ArrayList<>(); employees.add(new Employee(135, 10, "Simon", 56.7f, null)); - try { - EmpDeptTableFactory.THREAD_COLLECTION.set(employees); + try (final TryThreadLocal.Memo ignore = + EmpDeptTableFactory.THREAD_COLLECTION.push(employees)) { final CalciteAssert.AssertThat with = modelWithView( "select \"name\", \"empid\" as e, \"salary\" " + "from \"MUTABLE_EMPLOYEES\" where \"deptno\" = 10", @@ -316,8 +317,6 @@ public class JdbcTest { } } }); - } finally { - EmpDeptTableFactory.THREAD_COLLECTION.remove(); } } @@ -325,9 +324,8 @@ public class JdbcTest { @Test public void testModelWithInvalidModifiableView() throws Exception { final List<Employee> employees = new ArrayList<>(); employees.add(new Employee(135, 10, "Simon", 56.7f, null)); - try { - EmpDeptTableFactory.THREAD_COLLECTION.set(employees); - + try (final TryThreadLocal.Memo ignore = + EmpDeptTableFactory.THREAD_COLLECTION.push(employees)) { Util.discard(RESOURCE.noValueSuppliedForViewColumn(null, null)); modelWithView("select \"name\", \"empid\" as e, \"salary\" " + "from \"MUTABLE_EMPLOYEES\" where \"commission\" = 10", @@ -397,8 +395,6 @@ public class JdbcTest { null) .query("select \"name\" from \"adhoc\".V order by \"name\"") .runs(); - } finally { - EmpDeptTableFactory.THREAD_COLLECTION.remove(); } } @@ -871,67 +867,67 @@ public class JdbcTest { @Test public void testOnConnectionClose() throws Exception { final int[] closeCount = {0}; final int[] statementCloseCount = {0}; - HandlerDriver.HANDLERS.set( - new HandlerImpl() { - @Override public void - onConnectionClose(AvaticaConnection connection) { - ++closeCount[0]; - throw new RuntimeException(); - } - @Override public void onStatementClose(AvaticaStatement statement) { - ++statementCloseCount[0]; - throw new RuntimeException(); - } - }); - final HandlerDriver driver = - new HandlerDriver(); - CalciteConnection connection = (CalciteConnection) - driver.connect("jdbc:calcite:", new Properties()); - SchemaPlus rootSchema = connection.getRootSchema(); - rootSchema.add("hr", new ReflectiveSchema(new HrSchema())); - connection.setSchema("hr"); - final Statement statement = connection.createStatement(); - final ResultSet resultSet = - statement.executeQuery("select * from \"emps\""); - assertEquals(0, closeCount[0]); - assertEquals(0, statementCloseCount[0]); - resultSet.close(); - try { - resultSet.next(); - fail("resultSet.next() should throw SQLException when closed"); - } catch (SQLException e) { - assertThat(e.getMessage(), - containsString("next() called on closed cursor")); - } - assertEquals(0, closeCount[0]); - assertEquals(0, statementCloseCount[0]); + final HandlerImpl h = new HandlerImpl() { + @Override public void onConnectionClose(AvaticaConnection connection) { + ++closeCount[0]; + throw new RuntimeException(); + } - // Close statement. It throws SQLException, but statement is still closed. - try { - statement.close(); - fail("expecting error"); - } catch (SQLException e) { - // ok - } - assertEquals(0, closeCount[0]); - assertEquals(1, statementCloseCount[0]); + @Override public void onStatementClose(AvaticaStatement statement) { + ++statementCloseCount[0]; + throw new RuntimeException(); + } + }; + try (final TryThreadLocal.Memo ignore = + HandlerDriver.HANDLERS.push(h)) { + final HandlerDriver driver = new HandlerDriver(); + CalciteConnection connection = (CalciteConnection) + driver.connect("jdbc:calcite:", new Properties()); + SchemaPlus rootSchema = connection.getRootSchema(); + rootSchema.add("hr", new ReflectiveSchema(new HrSchema())); + connection.setSchema("hr"); + final Statement statement = connection.createStatement(); + final ResultSet resultSet = + statement.executeQuery("select * from \"emps\""); + assertEquals(0, closeCount[0]); + assertEquals(0, statementCloseCount[0]); + resultSet.close(); + try { + resultSet.next(); + fail("resultSet.next() should throw SQLException when closed"); + } catch (SQLException e) { + assertThat(e.getMessage(), + containsString("next() called on closed cursor")); + } + assertEquals(0, closeCount[0]); + assertEquals(0, statementCloseCount[0]); - // Close connection. It throws SQLException, but connection is still closed. - try { - connection.close(); - fail("expecting error"); - } catch (SQLException e) { - // ok - } - assertEquals(1, closeCount[0]); - assertEquals(1, statementCloseCount[0]); + // Close statement. It throws SQLException, but statement is still closed. + try { + statement.close(); + fail("expecting error"); + } catch (SQLException e) { + // ok + } + assertEquals(0, closeCount[0]); + assertEquals(1, statementCloseCount[0]); - // Close a closed connection. Handler is not called again. - connection.close(); - assertEquals(1, closeCount[0]); - assertEquals(1, statementCloseCount[0]); + // Close connection. It throws SQLException, but connection is still closed. + try { + connection.close(); + fail("expecting error"); + } catch (SQLException e) { + // ok + } + assertEquals(1, closeCount[0]); + assertEquals(1, statementCloseCount[0]); + + // Close a closed connection. Handler is not called again. + connection.close(); + assertEquals(1, closeCount[0]); + assertEquals(1, statementCloseCount[0]); - HandlerDriver.HANDLERS.remove(); + } } /** Tests {@link java.sql.Statement}.{@code closeOnCompletion()}. */ @@ -3335,15 +3331,18 @@ public class JdbcTest { /** Query that reads no columns from either underlying table. */ @Test public void testCountStar() { - CalciteAssert.hr() - .query("select count(*) c from \"hr\".\"emps\", \"hr\".\"depts\"") - .convertContains("LogicalAggregate(group=[{}], C=[COUNT()])\n" - + " LogicalProject(DUMMY=[0])\n" - + " LogicalJoin(condition=[true], joinType=[inner])\n" - + " LogicalProject(DUMMY=[0])\n" - + " EnumerableTableScan(table=[[hr, emps]])\n" - + " LogicalProject(DUMMY=[0])\n" - + " EnumerableTableScan(table=[[hr, depts]])"); + try (final TryThreadLocal.Memo ignored = Prepare.THREAD_TRIM.push(true); + final TryThreadLocal.Memo memo = Prepare.THREAD_EXPAND.push(true)) { + CalciteAssert.hr() + .query("select count(*) c from \"hr\".\"emps\", \"hr\".\"depts\"") + .convertContains("LogicalAggregate(group=[{}], C=[COUNT()])\n" + + " LogicalProject(DUMMY=[0])\n" + + " LogicalJoin(condition=[true], joinType=[inner])\n" + + " LogicalProject(DUMMY=[0])\n" + + " EnumerableTableScan(table=[[hr, emps]])\n" + + " LogicalProject(DUMMY=[0])\n" + + " EnumerableTableScan(table=[[hr, depts]])"); + } } /** Same result (and plan) as {@link #testSelectDistinct}. */ @@ -4173,26 +4172,25 @@ public class JdbcTest { /** Tests that field-trimming creates a project near the table scan. */ @Test public void testTrimFields() throws Exception { - try { - Prepare.THREAD_TRIM.set(true); + try (final TryThreadLocal.Memo ignored = Prepare.THREAD_TRIM.push(true)) { CalciteAssert.hr() .query("select \"name\", count(\"commission\") + 1\n" - + "from \"hr\".\"emps\"\n" - + "group by \"deptno\", \"name\"") + + "from \"hr\".\"emps\"\n" + + "group by \"deptno\", \"name\"") .convertContains("LogicalProject(name=[$1], EXPR$1=[+($2, 1)])\n" + " LogicalAggregate(group=[{0, 1}], agg#0=[COUNT($2)])\n" + " LogicalProject(deptno=[$1], name=[$2], commission=[$4])\n" + " EnumerableTableScan(table=[[hr, emps]])\n"); - } finally { - Prepare.THREAD_TRIM.set(false); } } /** Tests that field-trimming creates a project near the table scan, in a * query with windowed-aggregation. */ @Test public void testTrimFieldsOver() throws Exception { - try { - Prepare.THREAD_TRIM.set(true); + try (final TryThreadLocal.Memo ignored = Prepare.THREAD_TRIM.push(true); + final TryThreadLocal.Memo memo = Prepare.THREAD_EXPAND.push(true)) { + Util.discard(memo); + // The correct plan has a project on a filter on a project on a scan. CalciteAssert.hr() .query("select \"name\",\n" + " count(\"commission\") over (partition by \"deptno\") + 1\n" @@ -4203,8 +4201,6 @@ public class JdbcTest { + " LogicalFilter(condition=[>($0, 10)])\n" + " LogicalProject(empid=[$0], deptno=[$1], name=[$2], commission=[$4])\n" + " EnumerableTableScan(table=[[hr, emps]])\n"); - } finally { - Prepare.THREAD_TRIM.set(false); } } @@ -4220,9 +4216,10 @@ public class JdbcTest { "M=1", "M=1"); } + /** Tests multiple window aggregates over constants. * This tests that EnumerableWindowRel is able to reference the right slot - * when accessing constant for aggregation argument.*/ + * when accessing constant for aggregation argument. */ @Test public void testWinAggConstantMultipleConstants() { CalciteAssert.that() .with(CalciteAssert.Config.REGULAR) @@ -4644,7 +4641,9 @@ public class JdbcTest { * use as scratch space during development. */ // Do not add '@Ignore'; just remember not to commit changes to dummy.iq @Test public void testRunDummy() throws Exception { - checkRun("sql/dummy.iq"); + try (final TryThreadLocal.Memo ignored = Prepare.THREAD_TRIM.push(false)) { + checkRun("sql/dummy.iq"); + } } @Test public void testRunAgg() throws Exception { @@ -6478,8 +6477,8 @@ public class JdbcTest { /** Factory for EMP and DEPT tables. */ public static class EmpDeptTableFactory implements TableFactory<Table> { - public static final ThreadLocal<List<Employee>> THREAD_COLLECTION = - new ThreadLocal<>(); + public static final TryThreadLocal<List<Employee>> THREAD_COLLECTION = + TryThreadLocal.of(null); public Table create( SchemaPlus schema, @@ -6578,7 +6577,8 @@ public class JdbcTest { /** Mock driver that a given {@link Handler}. */ public static class HandlerDriver extends org.apache.calcite.jdbc.Driver { - private static final ThreadLocal<Handler> HANDLERS = new ThreadLocal<>(); + private static final TryThreadLocal<Handler> HANDLERS = + TryThreadLocal.of(null); public HandlerDriver() { } http://git-wip-us.apache.org/repos/asf/calcite/blob/c104c75b/core/src/test/java/org/apache/calcite/test/MaterializationTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/test/MaterializationTest.java b/core/src/test/java/org/apache/calcite/test/MaterializationTest.java index 65aabd7..05726b3 100644 --- a/core/src/test/java/org/apache/calcite/test/MaterializationTest.java +++ b/core/src/test/java/org/apache/calcite/test/MaterializationTest.java @@ -34,6 +34,7 @@ import org.apache.calcite.rex.RexUtil; import org.apache.calcite.runtime.Hook; import org.apache.calcite.sql.fun.SqlStdOperatorTable; import org.apache.calcite.util.JsonBuilder; +import org.apache.calcite.util.TryThreadLocal; import org.apache.calcite.util.Util; import org.apache.commons.lang3.StringUtils; @@ -127,8 +128,7 @@ public class MaterializationTest { } @Test public void testFilterQueryOnProjectView() { - try { - Prepare.THREAD_TRIM.set(true); + try (final TryThreadLocal.Memo ignored = Prepare.THREAD_TRIM.push(true)) { MaterializationService.setThreadLocal(); CalciteAssert.that() .withMaterializations( @@ -140,8 +140,6 @@ public class MaterializationTest { .enableMaterializations(true) .explainContains("EnumerableTableScan(table=[[hr, m0]])") .sameResultWithMaterializationsDisabled(); - } finally { - Prepare.THREAD_TRIM.set(false); } } @@ -155,8 +153,7 @@ public class MaterializationTest { * definition. */ private void checkMaterialize(String materialize, String query, String model, Function<ResultSet, Void> explainChecker) { - try { - Prepare.THREAD_TRIM.set(true); + try (final TryThreadLocal.Memo ignored = Prepare.THREAD_TRIM.push(true)) { MaterializationService.setThreadLocal(); CalciteAssert.that() .withMaterializations(model, "m0", materialize) @@ -164,8 +161,6 @@ public class MaterializationTest { .enableMaterializations(true) .explainMatches("", explainChecker) .sameResultWithMaterializationsDisabled(); - } finally { - Prepare.THREAD_TRIM.set(false); } } @@ -173,16 +168,13 @@ public class MaterializationTest { * definition. */ private void checkNoMaterialize(String materialize, String query, String model) { - try { - Prepare.THREAD_TRIM.set(true); + try (final TryThreadLocal.Memo ignored = Prepare.THREAD_TRIM.push(true)) { MaterializationService.setThreadLocal(); CalciteAssert.that() .withMaterializations(model, "m0", materialize) .query(query) .enableMaterializations(true) .explainContains("EnumerableTableScan(table=[[hr, emps]])"); - } finally { - Prepare.THREAD_TRIM.set(false); } } @@ -876,8 +868,7 @@ public class MaterializationTest { * Pre-populated materializations</a>. */ @Test public void testPrePopulated() { String q = "select \"deptno\" from \"emps\""; - try { - Prepare.THREAD_TRIM.set(true); + try (final TryThreadLocal.Memo ignored = Prepare.THREAD_TRIM.push(true)) { MaterializationService.setThreadLocal(); CalciteAssert.that() .withMaterializations( @@ -897,8 +888,6 @@ public class MaterializationTest { .enableMaterializations(true) .explainMatches("", CONTAINS_LOCATIONS) .sameResultWithMaterializationsDisabled(); - } finally { - Prepare.THREAD_TRIM.set(false); } } http://git-wip-us.apache.org/repos/asf/calcite/blob/c104c75b/core/src/test/java/org/apache/calcite/util/UtilTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/util/UtilTest.java b/core/src/test/java/org/apache/calcite/util/UtilTest.java index 78202e2..60122fb 100644 --- a/core/src/test/java/org/apache/calcite/util/UtilTest.java +++ b/core/src/test/java/org/apache/calcite/util/UtilTest.java @@ -73,6 +73,7 @@ import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.isA; import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.CoreMatchers.nullValue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; @@ -1472,6 +1473,42 @@ public class UtilTest { final String asString = Util.listToString(list); assertThat(Util.stringToList(asString), is(list)); } + + /** Tests {@link org.apache.calcite.util.TryThreadLocal}. + * + * <p>TryThreadLocal was introduced to fix + * <a href="https://issues.apache.org/jira/browse/CALCITE-915";>[CALCITE-915] + * Tests do not unset ThreadLocal values on exit</a>. */ + @Test public void testTryThreadLocal() { + final TryThreadLocal<String> local1 = TryThreadLocal.of("foo"); + assertThat(local1.get(), is("foo")); + TryThreadLocal.Memo memo1 = local1.push("bar"); + assertThat(local1.get(), is("bar")); + local1.set("baz"); + assertThat(local1.get(), is("baz")); + memo1.close(); + assertThat(local1.get(), is("foo")); + + final TryThreadLocal<String> local2 = TryThreadLocal.of(null); + assertThat(local2.get(), nullValue()); + TryThreadLocal.Memo memo2 = local2.push("a"); + assertThat(local2.get(), is("a")); + local2.set("b"); + assertThat(local2.get(), is("b")); + TryThreadLocal.Memo memo2B = local2.push(null); + assertThat(local2.get(), nullValue()); + memo2B.close(); + assertThat(local2.get(), is("b")); + memo2.close(); + assertThat(local2.get(), nullValue()); + + local2.set("x"); + try (TryThreadLocal.Memo ignore = local2.push("y")) { + assertThat(local2.get(), is("y")); + local2.set("z"); + } + assertThat(local2.get(), is("x")); + } } // End UtilTest.java
