Skip to content

Commit

Permalink
Fix findColumn and get*(String label) methods.
Browse files Browse the repository at this point in the history
The ResultSet `findColumn` methods (`BQForwardOnlyResultSet.findColumn`,
`BQScrollableResultSet.findColumn`, and `BQResultSet.findColumn`) called
`BQResultSetMetadata.getCatalogName` to determine the name of each column
instead of `BQResultSetMetadata.getColumnLabel`.

This was wrong because `getCatalogName` always returns the BigQuery project ID,
not the column name.

This bug in turn broke the get methods that are implemented in terms of
`findColumn`, e.g. `getString(String)`.

This change fixes `findColumn` and adds tests for methods that use it with some
caveats:

- `BQResultSet` has its findColumn fixed but it's not tested. That's because
its unused, even by its tests, which request a `TYPE_SCROLL_INSENSITIVE`
result set and thus get a `BQScrollableResultSet`.

- `BQScrollableResultSet` only accepts dates and times that can be parsed as
longs, which disagrees with the more widely used `BQForwardOnlyResultSet`.
Its date and time accessors are uncovered.

- `getBigDecimal` is excluded because it calls `BigDecimal.setScale`
without a rounding mode, which no longer works in modern Java. A
separate change can fix this.
  • Loading branch information
goomrw committed Dec 19, 2023
1 parent fd1165e commit 0e123bd
Show file tree
Hide file tree
Showing 6 changed files with 331 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,12 @@
import java.sql.Statement;
import java.sql.Time;
import java.sql.Timestamp;
import java.util.*;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import javax.annotation.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -593,12 +598,13 @@ public void deleteRow() throws SQLException {
*/
@Override
public int findColumn(String columnLabel) throws SQLException {
if (this.isClosed()) {
if (isClosed()) {
throw new BQSQLException("This Resultset is Closed");
}
int columncount = this.getMetaData().getColumnCount();
for (int i = 1; i <= columncount; i++) {
if (this.getMetaData().getCatalogName(i).equals(columnLabel)) {
final ResultSetMetaData metadata = this.getMetaData();
int columns = metadata.getColumnCount();
for (int i = 1; i <= columns; i++) {
if (metadata.getColumnLabel(i).equals(columnLabel)) {
return i;
}
}
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/net/starschema/clouddb/jdbc/BQResultSet.java
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,10 @@ public int findColumn(String columnLabel) throws SQLException {
if (this.isClosed()) {
throw new BQSQLException("This Resultset is Closed");
}
int columncount = this.getMetaData().getColumnCount();
for (int i = 1; i <= columncount; i++) {
if (this.getMetaData().getCatalogName(i).equals(columnLabel)) {
final ResultSetMetaData metadata = this.getMetaData();
int columns = metadata.getColumnCount();
for (int i = 1; i <= columns; i++) {
if (metadata.getColumnLabel(i).equals(columnLabel)) {
return i;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,10 @@ public int findColumn(String columnLabel) throws SQLException {
if (this.isClosed()) {
throw new BQSQLException("This Resultset is Closed");
}
int columncount = this.getMetaData().getColumnCount();
for (int i = 1; i <= columncount; i++) {
if (this.getMetaData().getCatalogName(i).equals(columnLabel)) {
final ResultSetMetaData metadata = this.getMetaData();
int columns = metadata.getColumnCount();
for (int i = 1; i <= columns; i++) {
if (metadata.getColumnLabel(i).equals(columnLabel)) {
return i;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.gson.Gson;
import java.io.IOException;
import java.math.BigDecimal;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.SQLException;
Expand Down Expand Up @@ -58,7 +59,7 @@
* @author Horváth Attila
* @author Gunics Balazs
*/
public class BQForwardOnlyResultSetFunctionTest {
public class BQForwardOnlyResultSetFunctionTest extends CommonTestsForResultSets {

private static java.sql.Connection con = null;
private java.sql.ResultSet resultForTest = null;
Expand Down Expand Up @@ -845,4 +846,10 @@ public void testStatelessQuery() throws SQLException {
Assertions.assertThat(bqResultSet.getJobId()).isNull();
Assertions.assertThat(bqResultSet.getQueryId()).contains("!");
}

@Override
protected Statement createStatementForCommonTests(final Connection connection)
throws SQLException {
return connection.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import com.google.api.client.testing.http.MockHttpTransport;
import com.google.api.client.testing.http.MockLowLevelHttpResponse;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.SQLException;
Expand All @@ -31,6 +32,7 @@
import org.assertj.core.api.Assertions;
import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -41,7 +43,7 @@
* @author Horváth Attila
* @author Gunics Balazs
*/
public class BQScrollableResultSetFunctionTest {
public class BQScrollableResultSetFunctionTest extends CommonTestsForResultSets {

private static java.sql.Connection con = null;
private static java.sql.ResultSet Result = null;
Expand Down Expand Up @@ -745,4 +747,31 @@ public void testStatelessQuery() throws SQLException {
Assertions.assertThat(bqResultSet.getJobId()).isNull();
Assertions.assertThat(bqResultSet.getQueryId()).contains("!");
}

@Override
protected Statement createStatementForCommonTests(Connection connection) throws SQLException {
return connection.createStatement(
ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_READ_ONLY);
}

@Override
@Test
@Ignore(
"Contradiction between BQSCrollableResultSet and BQForwardOnlyResultSet "
+ "dates and times: b/317107706")
public void testGetTimeByLabel() throws SQLException {}

@Override
@Test
@Ignore(
"Contradiction between BQSCrollableResultSet and BQForwardOnlyResultSet "
+ "dates and times: b/317107706")
public void testGetDateByLabel() throws SQLException {}

@Override
@Test
@Ignore(
"Contradiction between BQSCrollableResultSet and BQForwardOnlyResultSet "
+ "dates and times: b/317107706")
public void testGetTimestampByLabel() throws SQLException {}
}
Loading

0 comments on commit 0e123bd

Please sign in to comment.