Reading upper/lower bounds values with type promotions

[rambleraptor]

Part of #3270

Rationale for this change

Thanks to @ndrluis for the suggestion!

upper_bounds and lower_bounds fields are binary. The type they represent may not be the true type if a type promotion has occurred.

We need to determine the type based on the actual type + the number of bytes.

Are these changes tested?

Unit test included

Are there any user-facing changes?

[rambleraptor]

@kevinjqliu @geruh @Fokko please take a look when you can!

[geruh]

Thanks for raising Alex. I did a quick search and the same from bytes call is in some other places like StrictMetricsEval, and somewhere in inspect would they need this as well?

[rambleraptor]

@geruh thanks for the review! I added it into StrictMetricsEval as well.

I'm torn on inspect. There's value in a human understanding that the data on disk stating that the file is of a particular type. I didn't change it, but I'm happy to be overridden on that one.

[geruh]

I'm torn on inspect. There's value in a human understanding that the data on disk stating that the file is of a particular type. I didn't change it, but I'm happy to be overridden on that one.

I did a brief pass earlier so I wasn't able to give much context. Inspect methods are calling from_bytes for the bounds, and when type promotions come into play by that logic this will fail right. Because just like what you're laying down here, lower/upper_bound is the raw bytes from the manifest, and field.field_type comes from self.tbl.metadata.schema() which is the promoted type.

This can be proved by adding an integration test like:

schema = Schema(NestedField(1, "val", IntegerType(), required=False))

tbl = catalog.create_table("glue.dru", schema)

tbl.append(pa.table({"val": pa.array([10, 20, 30], type=pa.int32())}))

with tbl.update_schema() as upd:
    upd.update_column("val", field_type=LongType())
    
tbl.inspect.entries()  

running this gives me:

struct.error: unpack requires a buffer of 8 bytes

Arguably, this makes the conversions.py dispatch approach a little more appealing to me since we can fix it for everything in one place. but, would need to see or try it out. wdyt?

[geruh]

nit: just curious here when i linked the comment earlier to the java code I noticed they use < 8 any reason for the == 4 on these?

[rambleraptor]

If it's not 8 bytes, it's going to be 4 bytes. I'm not sure what kind of data would be 5-7 bytes. I changed it nonetheless just to match the Java implementation.

[geruh]

LGTM Thanks @rambleraptor!!