Skip to content

Commit 2f6182e

Browse files
olavloiteolavloite
authored
chore: fix float32 NaN comparison + deprecate generator (#2933)
The FLOAT32 type was only added to the com.google.cloud.spanner.RandomResultSetGenerator, but that generator misses multiple other types. The one in the .connection package should be used instead. This PR therefore deprecates the one that should not be used, and updates all tests that used the old one. It also adds FLOAT32 to the .connection.RandomResultSetGenerator, which is used by a larger number of tests. This discovered a bug in the FLOAT32 NaN comparison, which is also fixed in this PR. Note: The commit is not marked with 'fix:', as the feature has not yet been released.
1 parent cf865f8 commit 2f6182e

File tree

10 files changed

+70
-8
lines changed

10 files changed

+70
-8
lines changed

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1628,6 +1628,10 @@ void valueToString(StringBuilder b) {
16281628

16291629
@Override
16301630
boolean valueEquals(Value v) {
1631+
// NaN == NaN always returns false, so we need a custom check.
1632+
if (Float.isNaN(this.value)) {
1633+
return Float.isNaN(((Float32Impl) v).value);
1634+
}
16311635
return ((Float32Impl) v).value == value;
16321636
}
16331637

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/AsyncResultSetImplTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,9 +193,9 @@ public void withCallback() throws InterruptedException {
193193
});
194194
}
195195
finishedLatch.await();
196-
// There should be between 1 and 4 callbacks, depending on the timing of the threads.
196+
// There should be between 1 and 5 callbacks, depending on the timing of the threads.
197197
// Normally, there should be just 1 callback.
198-
assertThat(callbackCounter.get()).isIn(Range.closed(1, 4));
198+
assertThat(callbackCounter.get()).isIn(Range.closed(1, 5));
199199
assertThat(rowCounter.get()).isEqualTo(3);
200200
}
201201

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4083,6 +4083,7 @@ public void testGetAllTypesAsString() {
40834083
int col = 0;
40844084
assertAsString("true", resultSet, col++);
40854085
assertAsString("100", resultSet, col++);
4086+
assertAsString("-3.14", resultSet, col++);
40864087
assertAsString("3.14", resultSet, col++);
40874088
assertAsString("6.626", resultSet, col++);
40884089
assertAsString("test-string", resultSet, col++);
@@ -4100,6 +4101,15 @@ public void testGetAllTypesAsString() {
41004101
String.format("%d", Long.MAX_VALUE), String.format("%d", Long.MIN_VALUE), "NULL"),
41014102
resultSet,
41024103
col++);
4104+
assertAsString(
4105+
ImmutableList.of(
4106+
"NULL",
4107+
Float.valueOf(Float.MAX_VALUE).toString(),
4108+
Float.valueOf(Float.MIN_VALUE).toString(),
4109+
"NaN",
4110+
"3.14"),
4111+
resultSet,
4112+
col++);
41034113
assertAsString(ImmutableList.of("NULL", "-12345.6789", "3.14"), resultSet, col++);
41044114
assertAsString(ImmutableList.of("6.626", "NULL", "-8.9123"), resultSet, col++);
41054115
assertAsString(ImmutableList.of("test-string1", "NULL", "test-string2"), resultSet, col++);
@@ -4561,6 +4571,7 @@ private ListValue getRows(Dialect dialect) {
45614571
ListValue.newBuilder()
45624572
.addValues(com.google.protobuf.Value.newBuilder().setBoolValue(true).build())
45634573
.addValues(com.google.protobuf.Value.newBuilder().setStringValue("100").build())
4574+
.addValues(com.google.protobuf.Value.newBuilder().setNumberValue(-3.14f).build())
45644575
.addValues(com.google.protobuf.Value.newBuilder().setNumberValue(3.14d).build())
45654576
.addValues(com.google.protobuf.Value.newBuilder().setStringValue("6.626").build())
45664577
.addValues(com.google.protobuf.Value.newBuilder().setStringValue("test-string").build())
@@ -4609,6 +4620,31 @@ private ListValue getRows(Dialect dialect) {
46094620
.setNullValue(NullValue.NULL_VALUE)
46104621
.build())
46114622
.build()))
4623+
.addValues(
4624+
com.google.protobuf.Value.newBuilder()
4625+
.setListValue(
4626+
ListValue.newBuilder()
4627+
.addValues(
4628+
com.google.protobuf.Value.newBuilder()
4629+
.setNullValue(NullValue.NULL_VALUE)
4630+
.build())
4631+
.addValues(
4632+
com.google.protobuf.Value.newBuilder()
4633+
.setNumberValue(Float.MAX_VALUE)
4634+
.build())
4635+
.addValues(
4636+
com.google.protobuf.Value.newBuilder()
4637+
.setNumberValue(Float.MIN_VALUE)
4638+
.build())
4639+
.addValues(
4640+
com.google.protobuf.Value.newBuilder()
4641+
.setStringValue("NaN")
4642+
.build())
4643+
.addValues(
4644+
com.google.protobuf.Value.newBuilder()
4645+
.setNumberValue(3.14f)
4646+
.build())
4647+
.build()))
46124648
.addValues(
46134649
com.google.protobuf.Value.newBuilder()
46144650
.setListValue(

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/InlineBeginTransactionTest.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import com.google.cloud.spanner.MockSpannerServiceImpl.StatementResult;
3838
import com.google.cloud.spanner.TransactionRunner.TransactionCallable;
3939
import com.google.cloud.spanner.TransactionRunnerImpl.TransactionContextImpl;
40+
import com.google.cloud.spanner.connection.RandomResultSetGenerator;
4041
import com.google.common.collect.ImmutableList;
4142
import com.google.common.util.concurrent.MoreExecutors;
4243
import com.google.protobuf.AbstractMessage;

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/RandomResultSetGenerator.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@
3131
import com.google.spanner.v1.TypeCode;
3232
import java.util.Random;
3333

34+
/** @deprecated Use {@link com.google.cloud.spanner.connection.RandomResultSetGenerator} instead. */
35+
@Deprecated
3436
public class RandomResultSetGenerator {
3537
private static final Type[] TYPES =
3638
new Type[] {

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/ValueTest.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,7 @@ public void float32() {
193193
assertThat(v.getFloat32()).isWithin(0.0001f).of(1.23f);
194194
assertThat(v.toString()).isEqualTo("1.23");
195195
assertEquals("1.23", v.getAsString());
196+
assertEquals(Value.float32(Float.NaN), Value.float32(Float.NaN));
196197
}
197198

198199
@Test
@@ -224,6 +225,7 @@ public void float64() {
224225
assertThat(v.getFloat64()).isWithin(0.0001).of(1.23);
225226
assertThat(v.toString()).isEqualTo("1.23");
226227
assertEquals("1.23", v.getAsString());
228+
assertEquals(Value.float64(Double.NaN), Value.float64(Double.NaN));
227229
}
228230

229231
@Test
@@ -267,6 +269,7 @@ public void pgNumeric() {
267269
assertEquals(1234.5678D, value.getFloat64(), 0.00001);
268270
assertEquals("1234.5678", value.toString());
269271
assertEquals("1234.5678", value.getAsString());
272+
assertEquals(Value.pgNumeric("NaN"), Value.pgNumeric("NaN"));
270273
}
271274

272275
@Test

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractMockServerTest.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import com.google.cloud.spanner.ForceCloseSpannerFunction;
2020
import com.google.cloud.spanner.MockSpannerServiceImpl;
2121
import com.google.cloud.spanner.MockSpannerServiceImpl.StatementResult;
22-
import com.google.cloud.spanner.RandomResultSetGenerator;
2322
import com.google.cloud.spanner.Statement;
2423
import com.google.cloud.spanner.admin.database.v1.MockDatabaseAdminImpl;
2524
import com.google.cloud.spanner.admin.instance.v1.MockInstanceAdminImpl;

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/MergedResultSetTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ public void testAllResultsAreReturned() {
150150
if (numPartitions == 0) {
151151
assertEquals(0, resultSet.getColumnCount());
152152
} else {
153-
assertEquals(22, resultSet.getColumnCount());
153+
assertEquals(24, resultSet.getColumnCount());
154154
assertEquals(Type.bool(), resultSet.getColumnType(0));
155155
assertEquals(Type.bool(), resultSet.getColumnType("COL0"));
156156
assertEquals(10, resultSet.getColumnIndex("COL10"));

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/PartitionedQueryMockServerTest.java

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,15 +58,20 @@ public static Object[] data() {
5858

5959
@Parameter public Dialect dialect;
6060

61+
private Dialect currentDialect;
62+
6163
@Before
6264
public void setupDialect() {
63-
mockSpanner.putStatementResult(StatementResult.detectDialectResult(dialect));
65+
if (currentDialect != dialect) {
66+
mockSpanner.putStatementResult(StatementResult.detectDialectResult(dialect));
67+
SpannerPool.closeSpannerPool();
68+
currentDialect = dialect;
69+
}
6470
}
6571

6672
@After
6773
public void clearRequests() {
6874
mockSpanner.clearRequests();
69-
SpannerPool.closeSpannerPool();
7075
}
7176

7277
@Test
@@ -349,9 +354,9 @@ public void testRunEmptyPartitionedQuery() {
349354
statement, PartitionOptions.newBuilder().setMaxPartitions(maxPartitions).build())) {
350355
assertFalse(resultSet.next());
351356
assertNotNull(resultSet.getMetadata());
352-
assertEquals(22, resultSet.getMetadata().getRowType().getFieldsCount());
357+
assertEquals(24, resultSet.getMetadata().getRowType().getFieldsCount());
353358
assertNotNull(resultSet.getType());
354-
assertEquals(22, resultSet.getType().getStructFields().size());
359+
assertEquals(24, resultSet.getType().getStructFields().size());
355360
}
356361
}
357362
assertEquals(1, mockSpanner.countRequestsOfType(CreateSessionRequest.class));

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/RandomResultSetGenerator.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ public static Type[] generateAllTypes(Dialect dialect) {
5151
Arrays.asList(
5252
Type.newBuilder().setCode(TypeCode.BOOL).build(),
5353
Type.newBuilder().setCode(TypeCode.INT64).build(),
54+
Type.newBuilder().setCode(TypeCode.FLOAT32).build(),
5455
Type.newBuilder().setCode(TypeCode.FLOAT64).build(),
5556
dialect == Dialect.POSTGRESQL
5657
? Type.newBuilder()
@@ -76,6 +77,10 @@ public static Type[] generateAllTypes(Dialect dialect) {
7677
.setCode(TypeCode.ARRAY)
7778
.setArrayElementType(Type.newBuilder().setCode(TypeCode.INT64))
7879
.build(),
80+
Type.newBuilder()
81+
.setCode(TypeCode.ARRAY)
82+
.setArrayElementType(Type.newBuilder().setCode(TypeCode.FLOAT32))
83+
.build(),
7984
Type.newBuilder()
8085
.setCode(TypeCode.ARRAY)
8186
.setArrayElementType(Type.newBuilder().setCode(TypeCode.FLOAT64))
@@ -229,6 +234,13 @@ private void setRandomValue(Value.Builder builder, Type type) {
229234
random.nextInt(2019) + 1, random.nextInt(11) + 1, random.nextInt(28) + 1);
230235
builder.setStringValue(date.toString());
231236
break;
237+
case FLOAT32:
238+
if (randomNaN()) {
239+
builder.setNumberValue(Float.NaN);
240+
} else {
241+
builder.setNumberValue(random.nextFloat());
242+
}
243+
break;
232244
case FLOAT64:
233245
if (randomNaN()) {
234246
builder.setNumberValue(Double.NaN);

0 commit comments

Comments
 (0)