Repository navigation
Conversation
…nd dead STRUCT pass in QueryParameterValue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary & Bottleneck
QueryParameterValueandRangeperform several avoidable allocations during parameter construction, wire serialization (toValuePbandtoTypePb), and wire deserialization (fromPb):QueryParameterValue.fromPbunconditionally allocatesMap<String, QueryParameterType> parameterTypes = new HashMap<>()on every recursive call, including primitive scalars (INT64,STRING,BOOL, etc.) andARRAYelements whereparameterTypesis never used.type == StandardSQLTypeName.STRUCT,QueryParameterValue.fromPbiteratestypePb.getStructTypes()twice. The first pass allocates aHashMapandQueryParameterValueobjects and passes them tovalueBuilder.setStructTypes(structTypes). Immediately afterward, for every non-null struct,valueBuilder.setStructValues(structValues)callssetStructTypes(ImmutableMap.copyOf(structValues)), completely overwriting the map and allQueryParameterValueobjects constructed in the first pass.QueryParameterValue.Builder.setStructValuescallsImmutableMap.copyOf(structValues)twice on the same input map instead of sharing a singleImmutableMapinstance betweenstructTypesandstructValues.QueryParameterValue.array(T[], StandardSQLTypeName)allocatesnew ArrayList<>()with default capacity 10 instead ofarray.length.QueryParameterValue.toValuePb()andRange.getValues()callRange.getStart()andRange.getEnd()twice each. Every call toRange.getStart()orRange.getEnd()allocates a newFieldValuewrapper overgetStartInner()orgetEndInner().QueryParameterValue.validateTimestamp(String)allocates a regexMatcherforISO8601_TIMESTAMP_HIGH_PRECISION_PATTERN(\.(\d{10,})(?:\D|$)) on every timestamp string parameter, even when the fractional second has 9 or fewer digits.Changes
java-bigquery/google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/QueryParameterValue.java:parameterTypesonly inside the non-nullStandardSQLTypeName.STRUCTbranch offromPb, and only execute the placeholderstructTypesloop whenvaluePb == null || valuePb.getStructValues() == null.fromPb,toTypePb, andarray(T[], StandardSQLTypeName).ImmutableMap.copyOf(structValues)once inBuilder.setStructValuesand pass the resultingImmutableMapto bothsetStructTypesandsetStructValuesInner.getRangeValues().getStartInner()andgetEndInner()directly intoValuePb()instead of allocating 4FieldValuewrappers viagetStart()andgetEnd().ISO8601_TIMESTAMP_HIGH_PRECISION_PATTERN.matcher(timestamp)invalidateTimestampwithhasHighPrecisionFraction(timestamp)so timestamps with 9 or fewer fractional digits skipMatcherallocation.java-bigquery/google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/Range.java, readgetStartInner()andgetEndInner()directly ingetValues().java-bigquery/google-cloud-bigquery/src/test/java/com/google/cloud/bigquery/QueryParameterValueTest.java, assert thatstructValuesandstructTypesshare the sameImmutableMapinstance and verifyfromPbon nullSTRUCTvalues.Expected Impact
HashMapallocation per scalar, array element, and struct field inQueryParameterValue.fromPb.AutoValue_QueryParameterValueallocations, 1HashMap, and 1 duplicateImmutableMap.copyOfper non-nullSTRUCTparameter.FieldValueallocations perRANGEparameter duringtoValuePb()andRange.getValues().Matcherallocation for all standard (up to nanosecond precision) timestamp string parameters.Verification
QueryParameterValueTestandRangeTest:mvn test -pl java-bigquery/google-cloud-bigquery -Dtest=QueryParameterValueTest,RangeTest -Dclirr.skip=true -Denforcer.skip=true -Dflatten.skip=true -Dcheckstyle.skip=true -Dfmt.skip=true