Allow StringType and BytesType to have undefined byte lengths

Prior to this commit, Recap StringType and BytesType both required `bytes_` to
be set. If unset, a default of 65536 was used. This configuration allowed
developers to skip the `bytes` field in the concrete syntax tree (CST), but
still required `bytes_` to be set in the abstract syntax tree (AST). @adrianisk
and I converged on this design in
#284.

Recently, @mjperrone pointed out that requiring `bytes_` in the AST is still
awkward for JSON. In the absence of an undefined byte_ option, you have to set
*something* for JSON--either a magic number or INT/LONG_MAX. Thus far, we
defaulted to LONG_MAX. But a defined LONG_MAX and an undefined length are
actually two separate states. Allowing a converter to specify an undefined
string/byte length permits other converters to decide how to deal with the
undefined byte length.

I made the change and it fit quite naturally, so I think we're on the right
track with this. JSON and Hive metastore both benefited; both support undefined
lengths.

I've left the other converters alone for now. This means they'll continue to
barf if you go from JSON/HMS to Recap to Avro/Proto. I think that's fine for
now. We can revisit this later if we want to.

recap/converters/bigquery.py

@@ -23,9 +23,9 @@ def to_recap(self, fields: list[bigquery.SchemaField]) -> StructType:
                     # https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types
                     # Says "2 logical bytes + the UTF-8 encoded string size", so I'm assuming
                     # the 2 logical bytes are a uint16 length header, which is 65_536.
-                    field_type = StringType(bytes_=field.max_length or 65_536)
+                    field_type = StringType(bytes_=field.max_length)
                 case "BYTES":
-                    field_type = BytesType(bytes_=field.max_length or 65_536)
+                    field_type = BytesType(bytes_=field.max_length)
                 case "INT64" | "INTEGER" | "INT" | "SMALLINT" | "TINYINT" | "BYTEINT":
                     field_type = IntType(bits=64)
                 case "FLOAT" | "FLOAT64":

recap/converters/hive_metastore.py

@@ -67,8 +67,7 @@ def _parse_schema(self, htype: HType | HColumn) -> RecapType:
                 recap_type = NullType(**extra_attrs)
             case HPrimitiveType(primitive_type=PrimitiveCategory.STRING):
                 # TODO: Should handle multi-byte encodings
-                # Using 2^63-1 as the max length because Hive has no defined max.
-                recap_type = StringType(bytes_=9_223_372_036_854_775_807, **extra_attrs)
+                recap_type = StringType(**extra_attrs)
             case HPrimitiveType(primitive_type=PrimitiveCategory.BINARY):
                 recap_type = BytesType(bytes_=2_147_483_647, **extra_attrs)
             case HDecimalType(precision=int(precision), scale=int(scale)):

recap/converters/json_schema.py

@@ -79,30 +79,15 @@ def _parse(
                 values = self._parse(items, alias_strategy)
                 return ListType(values, **extra_attrs)
             case {"type": "string", "format": "bytes"}:
-                return BytesType(
-                    bytes_=9_223_372_036_854_775_807,
-                    **extra_attrs,
-                )
+                return BytesType(**extra_attrs)
             case {"type": "string", "format": "date"}:
-                return StringType(
-                    bytes_=9_223_372_036_854_775_807,
-                    logical="org.iso.8601.Date",
-                    **extra_attrs,
-                )
+                return StringType(logical="org.iso.8601.Date", **extra_attrs)
             case {"type": "string", "format": "date-time"}:
-                return StringType(
-                    bytes_=9_223_372_036_854_775_807,
-                    logical="org.iso.8601.DateTime",
-                    **extra_attrs,
-                )
+                return StringType(logical="org.iso.8601.DateTime", **extra_attrs)
             case {"type": "string", "format": "time"}:
-                return StringType(
-                    bytes_=9_223_372_036_854_775_807,
-                    logical="org.iso.8601.Time",
-                    **extra_attrs,
-                )
+                return StringType(logical="org.iso.8601.Time", **extra_attrs)
             case {"type": "string"}:
-                return StringType(bytes_=9_223_372_036_854_775_807, **extra_attrs)
+                return StringType(**extra_attrs)
             case {"type": "number"}:
                 return FloatType(bits=64, **extra_attrs)
             case {"type": "integer"}:

recap/types.py

@@ -106,7 +106,7 @@ def __eq__(self, other):
 
     def validate(self) -> None:
         if self.bits < 1 or self.bits > 2_147_483_647:
-            raise ValueError("bits must be between 1 and 2,147,483,647")
+            raise ValueError("Bits must be between 1 and 2,147,483,647")
 
 
 class FloatType(RecapType):
@@ -121,13 +121,13 @@ def __eq__(self, other):
 
     def validate(self) -> None:
         if self.bits < 1 or self.bits > 2_147_483_647:
-            raise ValueError("bits must be between 1 and 2,147,483,647")
+            raise ValueError("Bits must be between 1 and 2,147,483,647")
 
 
 class StringType(RecapType):
     """Represents a string Recap type."""
 
-    def __init__(self, bytes_: int = 65_536, variable: bool = True, **extra_attrs):
+    def __init__(self, bytes_: int | None = None, variable: bool = True, **extra_attrs):
         super().__init__("string", **extra_attrs)
         self.bytes_ = bytes_
         self.variable = variable
@@ -139,14 +139,18 @@ def __eq__(self, other):
         )
 
     def validate(self) -> None:
-        if self.bytes_ < 1 or self.bytes_ > 9_223_372_036_854_775_807:
-            raise ValueError("bytes must be between 1 and 9,223,372,036,854,775,807")
+        if not self.variable and self.bytes_ is None:
+            raise ValueError("Fixed length bytes must have a length set")
+        if self.bytes_ is not None and (
+            self.bytes_ < 1 or self.bytes_ > 9_223_372_036_854_775_807
+        ):
+            raise ValueError("Bytes must be between 1 and 9,223,372,036,854,775,807")
 
 
 class BytesType(RecapType):
     """Represents a bytes Recap type."""
 
-    def __init__(self, bytes_: int = 65_536, variable: bool = True, **extra_attrs):
+    def __init__(self, bytes_: int | None = None, variable: bool = True, **extra_attrs):
         super().__init__("bytes", **extra_attrs)
         self.bytes_ = bytes_
         self.variable = variable
@@ -158,8 +162,12 @@ def __eq__(self, other):
         )
 
     def validate(self) -> None:
-        if self.bytes_ < 1 or self.bytes_ > 9_223_372_036_854_775_807:
-            raise ValueError("bytes must be between 1 and 9,223,372,036,854,775,807")
+        if not self.variable and self.bytes_ is None:
+            raise ValueError("Fixed length bytes must have a length set")
+        if self.bytes_ is not None and (
+            self.bytes_ < 1 or self.bytes_ > 9_223_372_036_854_775_807
+        ):
+            raise ValueError("Bytes must be between 1 and 9,223,372,036,854,775,807")
 
 
 class ListType(RecapType):
@@ -187,7 +195,9 @@ def __eq__(self, other):
     def validate(self) -> None:
         if not self.variable and self.length is None:
             raise ValueError("Fixed length lists must have a length set")
-        if self.length and (self.length < 0 or self.length > 9_223_372_036_854_775_807):
+        if self.length is not None and (
+            self.length < 1 or self.length > 9_223_372_036_854_775_807
+        ):
             raise ValueError(
                 "List length must be between 0 and 9,223,372,036,854,775,807"
             )

tests/integration/clients/test_hive_metastore.py

@@ -312,7 +312,7 @@ def test_parameterized_types(hive_client):
     assert isinstance(fields[4].types[1].values, UnionType)
     assert isinstance(fields[4].types[1].values.types[0], NullType)
     assert isinstance(fields[4].types[1].values.types[1], StringType)
-    assert fields[4].types[1].values.types[1].bytes_ == 9_223_372_036_854_775_807
+    assert fields[4].types[1].values.types[1].bytes_ is None
     assert fields[4].doc == "c20"
 
     assert fields[5].extra_attrs["name"] == "col21"
@@ -330,7 +330,7 @@ def test_parameterized_types(hive_client):
     assert isinstance(fields[5].types[1].fields[1], UnionType)
     assert isinstance(fields[5].types[1].fields[1].types[0], NullType)
     assert isinstance(fields[5].types[1].fields[1].types[1], StringType)
-    assert fields[5].types[1].fields[1].types[1].bytes_ == 9_223_372_036_854_775_807
+    assert fields[5].types[1].fields[1].types[1].bytes_ is None
     assert fields[5].doc == "c21"
 
     assert fields[6].extra_attrs["name"] == "col22"
@@ -341,7 +341,7 @@ def test_parameterized_types(hive_client):
     assert fields[6].types[1].bits == 32
     assert fields[6].types[1].signed
     assert isinstance(fields[6].types[2], StringType)
-    assert fields[6].types[2].bytes_ == 9_223_372_036_854_775_807
+    assert fields[6].types[2].bytes_ is None
     assert fields[6].doc == "c22"
 
 

tests/spec/invalid/list_field_fixed_no_length.json → tests/spec/invalid/fixed_no_length.json

@@ -6,5 +6,13 @@
       "type": "float",
       "bits": 16
     }
+  },
+  {
+    "type": "string",
+    "variable": false
+  },
+  {
+    "type": "bytes",
+    "variable": false
   }
 ]

tests/spec/invalid/illegal_lengths.json

@@ -0,0 +1,59 @@
+[
+    {
+      "type": "list",
+      "variable": false,
+      "values": {
+        "type": "float",
+        "bits": 16
+      },
+      "length": -1
+    },
+    {
+      "type": "list",
+      "variable": false,
+      "values": {
+        "type": "float",
+        "bits": 16
+      },
+      "length": 0
+    },
+    {
+      "type": "list",
+      "variable": false,
+      "values": {
+        "type": "float",
+        "bits": 16
+      },
+      "length": 9223372036854775808
+    },
+    {
+      "type": "string",
+      "variable": false,
+      "bytes": -1
+    },
+    {
+      "type": "string",
+      "variable": false,
+      "bytes": 0
+    },
+    {
+      "type": "string",
+      "variable": false,
+      "bytes": 9223372036854775808
+    },
+    {
+      "type": "bytes",
+      "variable": false,
+      "bytes": -1
+    },
+    {
+      "type": "bytes",
+      "variable": false,
+      "bytes": 0
+    },
+    {
+      "type": "bytes",
+      "variable": false,
+      "bytes": 9223372036854775808
+    }
+  ]

tests/spec/test_json_schema_spec.py

@@ -20,7 +20,7 @@
 
 VALID_SCHEMA_DIR = "tests/spec/valid"
 INVALID_SCHEMA_DIR = "tests/spec/invalid"
-RECAP_SPEC_JSON_HTTP = "https://recap.build/specs/type/0.2.0.json"
+RECAP_SPEC_JSON_HTTP = "https://recap.build/specs/type/0.3.0.json"
 
 
 @pytest.fixture(scope="session")

tests/spec/valid/bytes_field.json

@@ -14,5 +14,12 @@
     "type": "bytes",
     "bytes": 9223372036854775807,
     "variable": true
+  },
+  {
+    "type": "bytes"
+  },
+  {
+    "type": "bytes",
+    "variable": true
   }
 ]

tests/spec/valid/string_field.json

@@ -14,5 +14,12 @@
     "type": "string",
     "bytes": 9223372036854775807,
     "variable": true
+  },
+  {
+    "type": "string"
+  },
+  {
+    "type": "string",
+    "variable": true
   }
 ]

tests/spec/valid/uuid_logical_field.json

@@ -10,10 +10,5 @@
     "logical": "build.recap.UUID",
     "variable": false,
     "bytes": 256
-  },
-  {
-    "type": "string",
-    "logical": "build.recap.UUID",
-    "variable": false
   }
 ]

tests/unit/clients/test_hive_metastore.py

@@ -106,7 +106,7 @@ class MockTable:
 
     assert isinstance(result.fields[7], UnionType)
     assert isinstance(result.fields[7].types[1], StringType)
-    assert result.fields[7].types[1].bytes_ == 9_223_372_036_854_775_807
+    assert result.fields[7].types[1].bytes_ is None
     assert result.fields[7].extra_attrs["name"] == "col8"
 
     assert isinstance(result.fields[8], UnionType)
@@ -301,7 +301,7 @@ class MockTable:
     # Validate sub_col2
     assert isinstance(struct_field.fields[1], UnionType)
     assert isinstance(struct_field.fields[1].types[1], StringType)
-    assert struct_field.fields[1].types[1].bytes_ == 9_223_372_036_854_775_807
+    assert struct_field.fields[1].types[1].bytes_ is None
     assert struct_field.fields[1].extra_attrs["name"] == "sub_col2"
 
 

tests/unit/converters/test_bigquery.py

@@ -8,8 +8,8 @@
 @pytest.mark.parametrize(
     "field_type,expected",
     [
-        ("STRING", StringType(bytes_=65_536, name="test_field")),
-        ("BYTES", BytesType(bytes_=65_536, name="test_field")),
+        ("STRING", StringType(name="test_field")),
+        ("BYTES", BytesType(name="test_field")),
         ("INT64", IntType(bits=64, name="test_field")),
         ("FLOAT", FloatType(bits=64, name="test_field")),
         ("BOOLEAN", BoolType(name="test_field")),
@@ -81,7 +81,7 @@ def test_record():
     expected = StructType(
         [
             IntType(bits=64, name="nested_int"),
-            StringType(bytes_=65_536, name="nested_string"),
+            StringType(bytes_=None, name="nested_string"),
         ],
         name="test_record",
     )

tests/unit/converters/test_hive_metastore.py

@@ -61,7 +61,7 @@
         (
             HPrimitiveType(primitive_type=PrimitiveCategory.STRING),
             UnionType(
-                [NullType(), StringType(bytes_=9_223_372_036_854_775_807)],
+                [NullType(), StringType()],
                 default=None,
                 name="test_col",
             ),
@@ -199,7 +199,7 @@
                     NullType(),
                     MapType(
                         keys=UnionType(
-                            [NullType(), StringType(bytes_=9_223_372_036_854_775_807)],
+                            [NullType(), StringType()],
                             default=None,
                         ),
                         values=UnionType(
@@ -241,7 +241,7 @@
                 [
                     NullType(),
                     IntType(bits=32),
-                    StringType(bytes_=9_223_372_036_854_775_807),
+                    StringType(),
                 ],
                 default=None,
                 name="test_col",
@@ -268,7 +268,7 @@
                             UnionType(
                                 [
                                     NullType(),
-                                    StringType(bytes_=9_223_372_036_854_775_807),
+                                    StringType(),
                                 ],
                                 default=None,
                                 name="field2",