ayushtkn commented on code in PR #6587:
URL: https://github.com/apache/hive/pull/6587#discussion_r3627713956


##########
jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/dao/TestDatabaseAccessorByDatabaseType.java:
##########
@@ -0,0 +1,535 @@
+/*
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");

Review Comment:
   Is this licence right? I think line-2 shouldn't be there, I think the 
content is also diffrent from othe rfiles
   ```
   /*
    * 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
    * <p/>
    * http://www.apache.org/licenses/LICENSE-2.0
    * <p/>
    * 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.
    */
   ```



##########
jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/dao/TestGenericJdbcDatabaseAccessor.java:
##########
@@ -246,6 +367,200 @@ public void testGetRecordIterator_invalidQuery() throws 
HiveJdbcDatabaseAccessEx
       JdbcRecordIterator iterator = accessor.getRecordIterator(conf, null, 
null, null, 0, 2);
   }
 
+  @Test
+  public void testGetBounds_minAndMax() throws HiveJdbcDatabaseAccessException 
{
+    Configuration conf = buildConfiguration();
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+    Pair<String, String> bounds = accessor.getBounds(conf, 
getStrategyIdColumnName(conf, accessor), true, true);
+
+    assertThat(bounds.getLeft(), is(equalTo("1")));
+    assertThat(bounds.getRight(), is(equalTo("5")));
+  }
+
+  @Test
+  public void testGetBounds_minOnly() throws HiveJdbcDatabaseAccessException {
+    Configuration conf = buildConfiguration();
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+    Pair<String, String> bounds = accessor.getBounds(conf, 
getStrategyIdColumnName(conf, accessor), true, false);
+
+    assertThat(bounds.getLeft(), is(equalTo("1")));
+    assertThat(bounds.getRight(), is(nullValue()));
+  }
+
+  @Test
+  public void testGetBounds_maxOnly() throws HiveJdbcDatabaseAccessException {
+    Configuration conf = buildConfiguration();
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+    Pair<String, String> bounds = accessor.getBounds(conf, 
getStrategyIdColumnName(conf, accessor), false, true);
+
+    assertThat(bounds.getLeft(), is(nullValue()));
+    assertThat(bounds.getRight(), is(equalTo("5")));
+  }
+
+  @Test
+  public void testAddBoundaryToQuery_bothBounds() {
+    GenericJdbcDatabaseAccessor accessor = new GenericJdbcDatabaseAccessor();
+    String result = accessor.addBoundaryToQuery(null, BOUNDARY_SQL, 
"strategy_id", "3", "5");
+
+    assertThat(result, is(equalTo(
+        "SELECT * FROM (" + BOUNDARY_SQL + ") tmptable WHERE \"strategy_id\" 
>= 3 AND \"strategy_id\" < 5")));
+  }
+
+  @Test
+  public void testAddBoundaryToQuery_upperOnlyIncludesNulls() {
+    GenericJdbcDatabaseAccessor accessor = new GenericJdbcDatabaseAccessor();
+    String result = accessor.addBoundaryToQuery(null, BOUNDARY_SQL, 
"strategy_id", null, "5");
+
+    assertThat(result, is(equalTo(
+        "SELECT * FROM (" + BOUNDARY_SQL + ") tmptable WHERE \"strategy_id\" < 
5 OR \"strategy_id\" IS NULL")));
+  }
+
+  @Test
+  public void testAddBoundaryToQuery_lowerOnly() {
+    GenericJdbcDatabaseAccessor accessor = new GenericJdbcDatabaseAccessor();
+    String result = accessor.addBoundaryToQuery(null, BOUNDARY_SQL, 
"strategy_id", "3", null);
+
+    assertThat(result, is(equalTo(
+        "SELECT * FROM (" + BOUNDARY_SQL + ") tmptable WHERE \"strategy_id\" 
>= 3")));
+  }
+
+  @Test
+  public void testAddBoundaryToQuery_rewritesNamedTable() {
+    GenericJdbcDatabaseAccessor accessor = new GenericJdbcDatabaseAccessor();
+    String result = accessor.addBoundaryToQuery("test_strategy",
+        "select * from test_strategy where priority > 0", "strategy_id", "3", 
"5");
+
+    assertThat(result, is(equalTo(
+        "select * from  (SELECT * FROM test_strategy WHERE \"strategy_id\" >= 
3 AND \"strategy_id\" < 5)"
+            + " tmptable  where priority > 0")));
+  }
+
+  @Test
+  public void testAddBoundaryToQuery_mysqlAccessorDoesNotQuoteColumn() {
+    MySqlDatabaseAccessor accessor = new MySqlDatabaseAccessor();
+    String result = accessor.addBoundaryToQuery(null, BOUNDARY_SQL, 
"strategy_id", "3", "5");
+
+    assertThat(result, is(equalTo(
+        "SELECT * FROM (" + BOUNDARY_SQL + ") tmptable WHERE strategy_id >= 3 
AND strategy_id < 5")));
+  }
+
+  @Test
+  public void testNeedColumnQuote_genericAccessor() {
+    Configuration conf = buildConfiguration();
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+
+    assertThat(accessor.needColumnQuote(), is(true));
+  }
+
+  @Test
+  public void testNeedColumnQuote_mysqlAccessor() {
+    Configuration conf = buildConfiguration();
+    conf.set(JdbcStorageConfig.DATABASE_TYPE.getPropertyName(), "MYSQL");
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+
+    assertThat(accessor.needColumnQuote(), is(false));
+  }
+
+  @Test
+  public void testClose_afterInitializedAccessor() throws Exception {
+    Configuration conf = buildConfiguration();
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+
+    assertThat(accessor.getColumnNames(conf), is(notNullValue()));
+    accessor.close();
+    accessor.close();
+  }
+
+  @Test
+  public void testGetRecordWriter_writesRows() throws Exception {
+    Configuration conf = buildConfiguration();
+    conf.set(JdbcStorageConfig.JDBC_URL.getPropertyName(),
+        conf.get(JdbcStorageConfig.JDBC_URL.getPropertyName())
+            .replace("jdbc:h2:mem:test;MODE=MySQL;INIT",
+                "jdbc:h2:mem:test_writer;MODE=MySQL;DB_CLOSE_DELAY=-1;INIT"));
+    conf.set(JdbcStorageConfig.TABLE.getPropertyName(), "test_writer");
+    conf.set(serdeConstants.LIST_COLUMNS, "id,name");
+    conf.set(serdeConstants.LIST_COLUMN_TYPES, "int,string");
+
+    TaskAttemptContext context = mock(TaskAttemptContext.class);
+    when(context.getConfiguration()).thenReturn(conf);
+
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+    @SuppressWarnings("rawtypes")
+    RecordWriter writer = accessor.getRecordWriter(context);
+    DBRecordWritable record = new DBRecordWritable(2);
+    record.set(0, 10);
+    record.set(1, "written");
+
+    writer.write(record, null);
+    writer.close(context);
+
+    String readBackUrl = 
conf.get(JdbcStorageConfig.JDBC_URL.getPropertyName()).replaceAll(";INIT=.*", 
"");
+    try (Connection conn = DriverManager.getConnection(readBackUrl);
+         Statement statement = conn.createStatement()) {
+      try (ResultSet rs = statement.executeQuery("select id, name from 
test_writer")) {
+        assertThat(rs.next(), is(true));
+        assertThat(rs.getInt("id"), is(equalTo(10)));
+        assertThat(rs.getString("name"), is(equalTo("written")));
+        assertThat(rs.next(), is(false));
+      }
+      statement.execute("SHUTDOWN");

Review Comment:
   this should have been in the finally block, if any assertion fails, this 
would leak, shutdown won't be called



##########
jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/dao/TestDatabaseAccessorByDatabaseType.java:
##########
@@ -0,0 +1,535 @@
+/*
+ *
+ * Licensed 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.hive.storage.jdbc.dao;
+
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.serde.serdeConstants;
+import org.apache.hive.storage.jdbc.conf.DatabaseType;
+import org.apache.hive.storage.jdbc.conf.JdbcStorageConfig;
+import org.apache.hive.storage.jdbc.exception.HiveJdbcDatabaseAccessException;
+
+import org.junit.Test;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+import java.sql.ResultSet;
+import java.sql.ResultSetMetaData;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.function.Function;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.equalToIgnoringCase;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+@RunWith(Enclosed.class)
+public class TestDatabaseAccessorByDatabaseType {
+
+  private static final String BASE = "select * from test_strategy";
+  private static final String H2_DRIVER = "org.h2.Driver";
+  private static final String R = OracleDatabaseAccessor.ROW_NUM_COLUMN_NAME;
+
+  @RunWith(Parameterized.class)
+  public static class AddLimitToQueryWithLimit {
+    private final DatabaseType databaseType;
+    private final String expectedQuery;
+
+    public AddLimitToQueryWithLimit(DatabaseType databaseType, String 
expectedQuery) {
+      this.databaseType = databaseType;
+      this.expectedQuery = expectedQuery;
+    }
+
+    @Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+      return queryRows(database -> database.limit2);
+    }
+
+    @Test
+    public void testAddLimitToQuery_withLimit() {
+      assertThat(accessor(databaseType).addLimitToQuery(BASE, 2), 
is(equalTo(expectedQuery)));
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  public static class AddLimitToQueryNoLimit {
+    private final DatabaseType databaseType;
+    private final String expectedQuery;
+
+    public AddLimitToQueryNoLimit(DatabaseType databaseType, String 
expectedQuery) {
+      this.databaseType = databaseType;
+      this.expectedQuery = expectedQuery;
+    }
+
+    @Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+      return queryRows(database -> database.noLimit);
+    }
+
+    @Test
+    public void testAddLimitToQuery_noLimit() {
+      assertThat(accessor(databaseType).addLimitToQuery(BASE, -1), 
is(equalTo(expectedQuery)));
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  public static class AddLimitAndOffsetToQueryLimitAndOffset {
+    private final DatabaseType databaseType;
+    private final String expectedQuery;
+
+    public AddLimitAndOffsetToQueryLimitAndOffset(DatabaseType databaseType, 
String expectedQuery) {
+      this.databaseType = databaseType;
+      this.expectedQuery = expectedQuery;
+    }
+
+    @Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+      return queryRows(database -> database.limitAndOffset);
+    }
+
+    @Test
+    public void testAddLimitAndOffsetToQuery_limitAndOffset() {
+      assertThat(accessor(databaseType).addLimitAndOffsetToQuery(BASE, 2, 1), 
is(equalTo(expectedQuery)));
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  public static class AddLimitAndOffsetToQueryOffsetOnly {
+    private final DatabaseType databaseType;
+    private final String expectedQuery;
+
+    public AddLimitAndOffsetToQueryOffsetOnly(DatabaseType databaseType, 
String expectedQuery) {
+      this.databaseType = databaseType;
+      this.expectedQuery = expectedQuery;
+    }
+
+    @Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+      return queryRows(database -> database.offsetOnly);
+    }
+
+    @Test
+    public void testAddLimitAndOffsetToQuery_offsetOnly() {
+      assertThat(accessor(databaseType).addLimitAndOffsetToQuery(BASE, -1, 2), 
is(equalTo(expectedQuery)));
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  public static class AddLimitAndOffsetToQueryOffsetZero {
+    private final DatabaseType databaseType;
+    private final String expectedQuery;
+
+    public AddLimitAndOffsetToQueryOffsetZero(DatabaseType databaseType, 
String expectedQuery) {
+      this.databaseType = databaseType;
+      this.expectedQuery = expectedQuery;
+    }
+
+    @Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+      return queryRows(database -> database.offsetZero);
+    }
+
+    @Test
+    public void testAddLimitAndOffsetToQuery_offsetZero() {
+      assertThat(accessor(databaseType).addLimitAndOffsetToQuery(BASE, 2, 0), 
is(equalTo(expectedQuery)));
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  public static class NeedColumnQuote {
+    private final DatabaseType databaseType;
+    private final boolean expectedNeedColumnQuote;
+
+    public NeedColumnQuote(DatabaseType databaseType, boolean 
expectedNeedColumnQuote) {
+      this.databaseType = databaseType;
+      this.expectedNeedColumnQuote = expectedNeedColumnQuote;
+    }
+
+    @Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+      return rowsFor(database -> database.needColumnQuote);
+    }
+
+    @Test
+    public void testNeedColumnQuote() {
+      assertThat(accessor(databaseType).needColumnQuote(), 
is(expectedNeedColumnQuote));
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  public static class ConstructQuery {
+    private final DatabaseType databaseType;
+    private final String expectedQuery;
+
+    public ConstructQuery(DatabaseType databaseType, String expectedQuery) {
+      this.databaseType = databaseType;
+      this.expectedQuery = expectedQuery;
+    }
+
+    @Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+      return queryRows(database -> database.constructQuery);
+    }
+
+    @Test
+    public void testConstructQuery() {
+      assertThat(accessor(databaseType).constructQuery("t", new String[]{"a", 
"b", "c"}),
+          is(equalTo(expectedQuery)));
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  public static class GetMetaDataQuery {
+    private final DatabaseType databaseType;
+    private final String expectedQuery;
+
+    public GetMetaDataQuery(DatabaseType databaseType, String expectedQuery) {
+      this.databaseType = databaseType;
+      this.expectedQuery = expectedQuery;
+    }
+
+    @Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+      return queryRows(database -> database.metaDataQuery);
+    }
+
+    @Test
+    public void testGetMetaDataQuery() {
+      assertThat(accessor(databaseType).getMetaDataQuery(BASE), 
is(equalTo(expectedQuery)));
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  public static class AccessorType {
+    private final DatabaseType databaseType;
+
+    public AccessorType(DatabaseType databaseType) {
+      this.databaseType = databaseType;
+    }
+
+    @Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+      return rowsFor(database -> null);
+    }
+
+    @Test
+    public void testAccessorType() {
+      
assertThat(DatabaseAccessorFactory.getAccessor(buildConfiguration(databaseType)),
+          instanceOf(accessorCase(databaseType).expectedType));
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  public static class FactoryMapping {
+    private final DatabaseType databaseType;
+    private final String driverClass;
+    private final Class<? extends DatabaseAccessor> expectedType;
+
+    public FactoryMapping(DatabaseType databaseType, String driverClass,
+        Class<? extends DatabaseAccessor> expectedType) {
+      this.databaseType = databaseType;
+      this.driverClass = driverClass;
+      this.expectedType = expectedType;
+    }
+
+    @Parameters(name = "{0}:{1}")
+    public static Collection<Object[]> data() {
+      return Arrays.asList(new Object[][]{
+          {DatabaseType.H2, "org.h2.Driver", 
GenericJdbcDatabaseAccessor.class},
+          {DatabaseType.METASTORE, "com.mysql.cj.jdbc.Driver", 
MySqlDatabaseAccessor.class},
+          {DatabaseType.METASTORE, "org.postgresql.Driver", 
PostgresDatabaseAccessor.class},
+          {DatabaseType.METASTORE, "oracle.jdbc.OracleDriver", 
OracleDatabaseAccessor.class},
+          {DatabaseType.METASTORE, 
"com.microsoft.sqlserver.jdbc.SQLServerDriver", MsSqlDatabaseAccessor.class},
+          {DatabaseType.METASTORE, "org.unknown.Driver", 
GenericJdbcDatabaseAccessor.class},
+      });
+    }
+
+    @Test
+    public void testFactoryMapping() {
+      Configuration config = new Configuration();
+      config.set(JdbcStorageConfig.DATABASE_TYPE.getPropertyName(), 
databaseType.name());
+      config.set(JdbcStorageConfig.JDBC_DRIVER_CLASS.getPropertyName(), 
driverClass);
+      assertThat(DatabaseAccessorFactory.getAccessor(config), 
instanceOf(expectedType));
+    }
+  }
+
+  public static class HiveColumnNames {
+    @Test
+    public void testGetColNamesFromRS_stripsQualifiedColumnNames() throws 
Exception {
+      ResultSet rs = mock(ResultSet.class);
+      ResultSetMetaData metadata = mock(ResultSetMetaData.class);
+      when(rs.getMetaData()).thenReturn(metadata);
+      when(metadata.getColumnCount()).thenReturn(3);
+      when(metadata.getColumnName(1)).thenReturn("test_strategy.strategy_id");
+      when(metadata.getColumnName(2)).thenReturn("name");
+      when(metadata.getColumnName(3)).thenReturn("db.schema.priority");
+
+      HiveDatabaseAccessor accessor = new HiveDatabaseAccessor();
+
+      assertThat(accessor.getColNamesFromRS(rs),
+          is(equalTo(Arrays.asList("strategy_id", "name", "priority"))));
+    }
+  }
+
+  @RunWith(Parameterized.class)
+  public static class H2CompatibleIntegration {
+    private final DatabaseType databaseType;
+
+    public H2CompatibleIntegration(DatabaseType databaseType) {
+      this.databaseType = databaseType;
+    }
+
+    @Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+      return Arrays.asList(new Object[][]{
+          {DatabaseType.MYSQL},
+          {DatabaseType.POSTGRES},
+          {DatabaseType.DB2},
+          {DatabaseType.DERBY},
+          {DatabaseType.HIVE},
+          {DatabaseType.JETHRO_DATA},
+      });
+    }
+
+    @Test
+    public void testGetRecordIterator_limitTwo() throws 
HiveJdbcDatabaseAccessException {
+      Configuration conf = buildIntegrationConfiguration(databaseType);
+      DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+      JdbcRecordIterator iterator = accessor.getRecordIterator(conf, null, 
null, null, 2, 0);
+
+      assertThat(iterator, is(notNullValue()));
+      int count = 0;
+      while (iterator.hasNext()) {
+        iterator.next();
+        count++;
+      }
+      assertThat(count, is(equalTo(2)));
+      iterator.close();
+    }
+
+    @Test
+    public void testGetColumnNames() throws HiveJdbcDatabaseAccessException {
+      Configuration conf = buildIntegrationConfiguration(databaseType);
+      DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+      List<String> columnNames = accessor.getColumnNames(conf);
+
+      assertThat(columnNames, is(notNullValue()));
+      assertThat(columnNames.size(), is(equalTo(7)));
+      assertThat(columnNames.get(0), is(equalToIgnoringCase("strategy_id")));
+    }
+
+    @Test
+    public void testGetBounds_minAndMax() throws 
HiveJdbcDatabaseAccessException {
+      Configuration conf = buildIntegrationConfiguration(databaseType);
+      DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+      Pair<String, String> bounds = accessor.getBounds(conf, 
accessor.getColumnNames(conf).get(0), true, true);
+
+      assertThat(bounds.getLeft(), is(equalTo("1")));
+      assertThat(bounds.getRight(), is(equalTo("5")));
+    }
+  }
+
+  private static final AccessorCase[] CASES = {
+      db(DatabaseType.MYSQL, MySqlDatabaseAccessor.class,
+          BASE + " LIMIT 2",
+          BASE,
+          BASE + " LIMIT 1,2",
+          BASE,
+          BASE + " LIMIT 2",
+          false,
+          "INSERT INTO t VALUES (?,?,?);",
+          BASE + " LIMIT 1"),
+      db(DatabaseType.POSTGRES, PostgresDatabaseAccessor.class,
+          BASE + " LIMIT 2",
+          BASE,
+          BASE + " LIMIT 2 OFFSET 1",
+          BASE,
+          BASE + " LIMIT 2",
+          true,
+          "INSERT INTO t VALUES (?,?,?);",
+          BASE + " LIMIT 1"),
+      db(DatabaseType.DB2, DB2DatabaseAccessor.class,
+          BASE + " LIMIT 2",
+          BASE,
+          BASE + " LIMIT 2 OFFSET 1",
+          BASE,
+          BASE + " LIMIT 2",
+          true,
+          "INSERT INTO t VALUES (?,?,?)",
+          BASE + " LIMIT 1"),
+      db(DatabaseType.DERBY, DerbyDatabaseAccessor.class,
+          BASE + " {LIMIT 2}",
+          BASE,
+          BASE + " {LIMIT 2 OFFSET 1}",
+          BASE + " {OFFSET 2}",
+          BASE + " {LIMIT 2}",
+          true,
+          "INSERT INTO t VALUES (?,?,?)",
+          BASE + " {LIMIT 1}"),
+      db(DatabaseType.MSSQL, MsSqlDatabaseAccessor.class,
+          BASE + " {LIMIT 2}",
+          BASE,
+          BASE + " ORDER BY 1 OFFSET 1 ROWS FETCH NEXT 2 ROWS ONLY",
+          BASE,
+          BASE + " {LIMIT 2}",
+          true,
+          "INSERT INTO t VALUES (?,?,?);",
+          BASE + " {LIMIT 1}"),
+      db(DatabaseType.ORACLE, OracleDatabaseAccessor.class,
+          "SELECT * FROM (" + BASE + ") WHERE ROWNUM <= 2",
+          BASE,
+          "SELECT * FROM (SELECT t.*, ROWNUM AS " + R + " FROM (" + BASE + ") 
t) WHERE "
+              + R + " >1 AND " + R + " <=3",
+          BASE,
+          "SELECT * FROM (" + BASE + ") WHERE ROWNUM <= 2",
+          true,
+          "INSERT INTO t VALUES (?,?,?)",
+          "SELECT * FROM (" + BASE + ") WHERE ROWNUM <= 1"),
+      db(DatabaseType.HIVE, HiveDatabaseAccessor.class,
+          BASE + " LIMIT 2",
+          BASE,
+          BASE + " LIMIT 2 OFFSET 1",
+          BASE,
+          BASE + " LIMIT 2",
+          true,
+          "INSERT INTO t VALUES (?,?,?);",
+          BASE + " LIMIT 0"),
+      db(DatabaseType.JETHRO_DATA, JethroDatabaseAccessor.class,
+          "Select * from (" + BASE + ") as \"tmp\" limit 2",
+          "Select * from (" + BASE + ") as \"tmp\" limit -1",
+          BASE + " LIMIT 1,2",
+          BASE + " LIMIT 2,-1",
+          "Select * from (" + BASE + ") as \"tmp\" limit 2",
+          true,
+          "INSERT INTO t VALUES (?,?,?);",
+          "Select * from (" + BASE + ") as \"tmp\" limit 0")
+  };
+
+  private static Object[] row(Object... values) {
+    return values;

Review Comment:
   what is this for?



##########
jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/dao/TestGenericJdbcDatabaseAccessor.java:
##########
@@ -246,6 +367,200 @@ public void testGetRecordIterator_invalidQuery() throws 
HiveJdbcDatabaseAccessEx
       JdbcRecordIterator iterator = accessor.getRecordIterator(conf, null, 
null, null, 0, 2);
   }
 
+  @Test
+  public void testGetBounds_minAndMax() throws HiveJdbcDatabaseAccessException 
{
+    Configuration conf = buildConfiguration();
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+    Pair<String, String> bounds = accessor.getBounds(conf, 
getStrategyIdColumnName(conf, accessor), true, true);
+
+    assertThat(bounds.getLeft(), is(equalTo("1")));
+    assertThat(bounds.getRight(), is(equalTo("5")));
+  }
+
+  @Test
+  public void testGetBounds_minOnly() throws HiveJdbcDatabaseAccessException {
+    Configuration conf = buildConfiguration();
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+    Pair<String, String> bounds = accessor.getBounds(conf, 
getStrategyIdColumnName(conf, accessor), true, false);
+
+    assertThat(bounds.getLeft(), is(equalTo("1")));
+    assertThat(bounds.getRight(), is(nullValue()));
+  }
+
+  @Test
+  public void testGetBounds_maxOnly() throws HiveJdbcDatabaseAccessException {
+    Configuration conf = buildConfiguration();
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+    Pair<String, String> bounds = accessor.getBounds(conf, 
getStrategyIdColumnName(conf, accessor), false, true);
+
+    assertThat(bounds.getLeft(), is(nullValue()));
+    assertThat(bounds.getRight(), is(equalTo("5")));
+  }
+
+  @Test
+  public void testAddBoundaryToQuery_bothBounds() {
+    GenericJdbcDatabaseAccessor accessor = new GenericJdbcDatabaseAccessor();
+    String result = accessor.addBoundaryToQuery(null, BOUNDARY_SQL, 
"strategy_id", "3", "5");
+
+    assertThat(result, is(equalTo(
+        "SELECT * FROM (" + BOUNDARY_SQL + ") tmptable WHERE \"strategy_id\" 
>= 3 AND \"strategy_id\" < 5")));
+  }
+
+  @Test
+  public void testAddBoundaryToQuery_upperOnlyIncludesNulls() {
+    GenericJdbcDatabaseAccessor accessor = new GenericJdbcDatabaseAccessor();
+    String result = accessor.addBoundaryToQuery(null, BOUNDARY_SQL, 
"strategy_id", null, "5");
+
+    assertThat(result, is(equalTo(
+        "SELECT * FROM (" + BOUNDARY_SQL + ") tmptable WHERE \"strategy_id\" < 
5 OR \"strategy_id\" IS NULL")));
+  }
+
+  @Test
+  public void testAddBoundaryToQuery_lowerOnly() {
+    GenericJdbcDatabaseAccessor accessor = new GenericJdbcDatabaseAccessor();
+    String result = accessor.addBoundaryToQuery(null, BOUNDARY_SQL, 
"strategy_id", "3", null);
+
+    assertThat(result, is(equalTo(
+        "SELECT * FROM (" + BOUNDARY_SQL + ") tmptable WHERE \"strategy_id\" 
>= 3")));
+  }
+
+  @Test
+  public void testAddBoundaryToQuery_rewritesNamedTable() {
+    GenericJdbcDatabaseAccessor accessor = new GenericJdbcDatabaseAccessor();
+    String result = accessor.addBoundaryToQuery("test_strategy",
+        "select * from test_strategy where priority > 0", "strategy_id", "3", 
"5");
+
+    assertThat(result, is(equalTo(
+        "select * from  (SELECT * FROM test_strategy WHERE \"strategy_id\" >= 
3 AND \"strategy_id\" < 5)"
+            + " tmptable  where priority > 0")));
+  }
+
+  @Test
+  public void testAddBoundaryToQuery_mysqlAccessorDoesNotQuoteColumn() {
+    MySqlDatabaseAccessor accessor = new MySqlDatabaseAccessor();
+    String result = accessor.addBoundaryToQuery(null, BOUNDARY_SQL, 
"strategy_id", "3", "5");
+
+    assertThat(result, is(equalTo(
+        "SELECT * FROM (" + BOUNDARY_SQL + ") tmptable WHERE strategy_id >= 3 
AND strategy_id < 5")));
+  }
+
+  @Test
+  public void testNeedColumnQuote_genericAccessor() {
+    Configuration conf = buildConfiguration();
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+
+    assertThat(accessor.needColumnQuote(), is(true));
+  }
+
+  @Test
+  public void testNeedColumnQuote_mysqlAccessor() {
+    Configuration conf = buildConfiguration();
+    conf.set(JdbcStorageConfig.DATABASE_TYPE.getPropertyName(), "MYSQL");
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+
+    assertThat(accessor.needColumnQuote(), is(false));
+  }
+
+  @Test
+  public void testClose_afterInitializedAccessor() throws Exception {
+    Configuration conf = buildConfiguration();
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+
+    assertThat(accessor.getColumnNames(conf), is(notNullValue()));
+    accessor.close();
+    accessor.close();
+  }
+
+  @Test
+  public void testGetRecordWriter_writesRows() throws Exception {
+    Configuration conf = buildConfiguration();
+    conf.set(JdbcStorageConfig.JDBC_URL.getPropertyName(),
+        conf.get(JdbcStorageConfig.JDBC_URL.getPropertyName())
+            .replace("jdbc:h2:mem:test;MODE=MySQL;INIT",
+                "jdbc:h2:mem:test_writer;MODE=MySQL;DB_CLOSE_DELAY=-1;INIT"));
+    conf.set(JdbcStorageConfig.TABLE.getPropertyName(), "test_writer");
+    conf.set(serdeConstants.LIST_COLUMNS, "id,name");
+    conf.set(serdeConstants.LIST_COLUMN_TYPES, "int,string");
+
+    TaskAttemptContext context = mock(TaskAttemptContext.class);
+    when(context.getConfiguration()).thenReturn(conf);
+
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+    @SuppressWarnings("rawtypes")
+    RecordWriter writer = accessor.getRecordWriter(context);
+    DBRecordWritable record = new DBRecordWritable(2);
+    record.set(0, 10);
+    record.set(1, "written");
+
+    writer.write(record, null);
+    writer.close(context);
+
+    String readBackUrl = 
conf.get(JdbcStorageConfig.JDBC_URL.getPropertyName()).replaceAll(";INIT=.*", 
"");
+    try (Connection conn = DriverManager.getConnection(readBackUrl);
+         Statement statement = conn.createStatement()) {
+      try (ResultSet rs = statement.executeQuery("select id, name from 
test_writer")) {
+        assertThat(rs.next(), is(true));
+        assertThat(rs.getInt("id"), is(equalTo(10)));
+        assertThat(rs.getString("name"), is(equalTo("written")));
+        assertThat(rs.next(), is(false));
+      }
+      statement.execute("SHUTDOWN");
+    }
+  }
+
+  @Test
+  public void testGetRecordWriter_writesMultipleRows() throws Exception {
+    Configuration conf = buildConfiguration();
+    conf.set(JdbcStorageConfig.JDBC_URL.getPropertyName(),
+        conf.get(JdbcStorageConfig.JDBC_URL.getPropertyName())
+            .replace("jdbc:h2:mem:test;MODE=MySQL;INIT",
+                
"jdbc:h2:mem:test_writer_multi;MODE=MySQL;DB_CLOSE_DELAY=-1;INIT"));
+    conf.set(JdbcStorageConfig.TABLE.getPropertyName(), "test_writer");
+    conf.set(serdeConstants.LIST_COLUMNS, "id,name");
+    conf.set(serdeConstants.LIST_COLUMN_TYPES, "int,string");
+
+    TaskAttemptContext context = mock(TaskAttemptContext.class);
+    when(context.getConfiguration()).thenReturn(conf);
+
+    DatabaseAccessor accessor = DatabaseAccessorFactory.getAccessor(conf);
+    @SuppressWarnings("rawtypes")
+    RecordWriter writer = accessor.getRecordWriter(context);
+
+    DBRecordWritable firstRecord = new DBRecordWritable(2);
+    firstRecord.set(0, 11);
+    firstRecord.set(1, "first");
+    writer.write(firstRecord, null);
+
+    DBRecordWritable secondRecord = new DBRecordWritable(2);
+    secondRecord.set(0, 12);
+    secondRecord.set(1, "second");
+    writer.write(secondRecord, null);
+
+    writer.close(context);
+
+    String readBackUrl = 
conf.get(JdbcStorageConfig.JDBC_URL.getPropertyName()).replaceAll(";INIT=.*", 
"");
+    try (Connection conn = DriverManager.getConnection(readBackUrl);
+         Statement statement = conn.createStatement()) {
+      try (ResultSet rs = statement.executeQuery("select id, name from 
test_writer order by id")) {
+        assertThat(rs.next(), is(true));
+        assertThat(rs.getInt("id"), is(equalTo(11)));
+        assertThat(rs.getString("name"), is(equalTo("first")));
+
+        assertThat(rs.next(), is(true));
+        assertThat(rs.getInt("id"), is(equalTo(12)));
+        assertThat(rs.getString("name"), is(equalTo("second")));
+
+        assertThat(rs.next(), is(false));
+      }
+      statement.execute("SHUTDOWN");

Review Comment:
   same as above, it should be in finally or maybe the cleanup should be in 
`After` block



-- 
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]

Reply via email to