Refactored client library code for Data Types for simplicity and code reuse #1

gunjan-juyal · 2023-09-25T15:29:24Z

Refactored client library code for Data Types for simplicity and code reuse

This refactoring is attempting the following benefits:

Code reuse for common value encoding conversions
Reducing the number of places when adding a new data-type, especially the type-related conversion logic.
Refactor the existing primitive types to use the new helper functions to simplify existing code and tests.

Reference - code change required for adding a new type:

Fixes - (Bug to be created) ☕️

[Note: This PR is raised on a fork of the java-spanner client repo. It is currently a prototype, and once things look promising a separate PR will be raised for merging this to the official repo.]

…-reuse. This refactoring is attempting the following benefits: 1. Code reuse for common value encoding conversions 2. Reducing the number of places when adding a new data-type, especially the type-related conversion logic. 3. Refactor the existing primitive types to use the new helper functions to simplify existing code.

charvisingla · 2023-09-27T06:34:22Z

These changes are great for a first pass.
I think the Type specific logic is still essentially scattered all over the place for non-primitive or special handling. A developer would still need to look at all the areas to figure out whether they need to add a special case etc. Can we make generic interfaces to be overriden for new types to limit all set of specialization logic for a new type in 1-2 files? Say in either Type and Value classes? or a new generic interface that covers all methods required for type specific logics with a default implementation for each?

… set, setting query parameters and mutations

thiagotnunes

One of the ideas was to centralize Type specific logic such that when we add a new type we would only need to touch one file, maybe two files. I see that if I add a new type I would still need to change: potentially Type.java, TypeHelper.java, Value.java and potentially ChecksumResultSet.java.

Could we see if we can centralize this further?

Perhaps all of the following could live in a TypeMapper of sorts:

Code to primitive type
Code to array type
Supported primitive type classes
Extracting value using code
Extracting array using code

thiagotnunes · 2023-10-12T21:41:42Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/Type.java

-        return date();
+    Code typeCode = Code.fromProto(proto.getCode(), proto.getTypeAnnotation());
+    if (isPrimitiveTypeCodeSupported(typeCode)) {
+      System.out.format("Type from map: %s\n", CODE_TO_PRIMITIVE_TYPE.get(typeCode));


Yes, thanks for catching. Will clean up

thiagotnunes · 2023-10-12T21:43:30Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/TypeHelper.java

+import com.google.cloud.spanner.Type.Code;
+import java.util.List;
+
+public final class TypeHelper {


nit: *Helper is too generic of a name, perhaps PrimitiveTypeMapper or something alike?

I agree. Naming is hard! Will think of an alternative, otherwise Mapper sounds good.
@thiagotnunes I have named the types supported by these generic methods/interfaces as "Primitive" types, since I do not intend to handle complex types such as Struct or Protos. I am not sure if this is indicative though. Do comment if this sounds alright, or if you can think of alternatives

gunjan-juyal · 2023-10-13T06:24:05Z

These changes are great for a first pass. I think the Type specific logic is still essentially scattered all over the place for non-primitive or special handling. A developer would still need to look at all the areas to figure out whether they need to add a special case etc. Can we make generic interfaces to be overriden for new types to limit all set of specialization logic for a new type in 1-2 files? Say in either Type and Value classes? or a new generic interface that covers all methods required for type specific logics with a default implementation for each?

@charvisingla Yes, in the first parse I tried to just identify and reduce the duplication wherever I found common code fragments. Special cases are still left untouched.

In a second parse I am now looking at these two as you suggested:

Limiting all type-specific transcoding, casting etc to 1 or 2 places
Introduce a default implementation that is sufficient for future simple types (e.g. Number-compatible types such as INT32 and String-compatible types such as JSON/JSONB in the past), and only custom logic needs to override this.

I had faced the following challenges in phase-1 while exploring ways to add new generic interfaces. Do share if you have any specific ideas or suggestions:

Public interfaces: We should not break or deprecate any existing interfaces, and prefer to support existing conventions for future types - e.g. if we have a ValueBinder.to(boolean) for Boolean type then we should later add a ValueBinder.to(int) for INT32.
Private or internal interfaces: The volume of existing work is huge, and much of this is tightly coupled to specific types. E.g. separate method signatures for each data type.

gunjan-juyal · 2023-10-13T10:35:48Z

One of the ideas was to centralize Type specific logic such that when we add a new type we would only need to touch one file, maybe two files. I see that if I add a new type I would still need to change: potentially Type.java, TypeHelper.java, Value.java and potentially ChecksumResultSet.java.

Could we see if we can centralize this further?

Perhaps all of the following could live in a TypeMapper of sorts:

Code to primitive type

Code to array type

Supported primitive type classes

Extracting value using code

Extracting array using code

Yes, and also (6) Checksum calculation for each type. Attempting this now.

gunjan-juyal self-assigned this Sep 26, 2023

gunjan-juyal marked this pull request as draft September 26, 2023 05:27

Added test cases for generic public API usage for reading from result…

18a4e86

… set, setting query parameters and mutations

thiagotnunes reviewed Oct 12, 2023

View reviewed changes

Merged changes from main

7ccfeb2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored client library code for Data Types for simplicity and code reuse #1

Refactored client library code for Data Types for simplicity and code reuse #1

gunjan-juyal commented Sep 25, 2023 •

edited

charvisingla commented Sep 27, 2023

thiagotnunes left a comment

thiagotnunes Oct 12, 2023

gunjan-juyal Oct 13, 2023

thiagotnunes Oct 12, 2023

gunjan-juyal Oct 13, 2023

gunjan-juyal commented Oct 13, 2023

gunjan-juyal commented Oct 13, 2023

Refactored client library code for Data Types for simplicity and code reuse #1

Are you sure you want to change the base?

Refactored client library code for Data Types for simplicity and code reuse #1

Conversation

gunjan-juyal commented Sep 25, 2023 • edited

charvisingla commented Sep 27, 2023

thiagotnunes left a comment

Choose a reason for hiding this comment

thiagotnunes Oct 12, 2023

Choose a reason for hiding this comment

gunjan-juyal Oct 13, 2023

Choose a reason for hiding this comment

thiagotnunes Oct 12, 2023

Choose a reason for hiding this comment

gunjan-juyal Oct 13, 2023

Choose a reason for hiding this comment

gunjan-juyal commented Oct 13, 2023

gunjan-juyal commented Oct 13, 2023

gunjan-juyal commented Sep 25, 2023 •

edited