Python: Fix repr of the Literals

[Fokko]

Feature Request / Improvement

We instantiate the literals using the literal constructor, but the repr is the specific literal. I think we just want to use the literal constructor in the repr as well:

def test_double_literal():
>       assert repr(literal(12.34)) == 'literal(12.34)'
E       AssertionError: assert 'DoubleLiteral(12.34)' == 'literal(12.34)'
E         - literal(12.34)
E         + DoubleLiteral(12.34)
E         ? ++++ ++

Would be great if we can override the __repr__ of each of the literals, and add tests to make sure that they stay correct.

Query engine

No response

[hendrikmakait]

I'm concerned about the introduction of ambiguity to the __repr__ with this change. Namely, the __repr__ of both a DoubleLiteral(1.0) and a FloatLiteral(1.0) would become literal(1.0), which in turn evaluates to a DoubleLiteral(1.0). Hence, the __repr__ becomes ambiguous with respect to the class of the original object and it will not recreate the same object for the case of the FloatLiteral. A similar situation occurs for BinaryLiteral and FixedLiteral.

[rdblue]

@hendrikmakait makes a good point, although I think double and float are the only cases where it may matter. And even then, binding should handle any problems caused by this. Isn't binding the only way to produce FloatLiteral anyway?

[Fokko]

That's a very good point indeed. I think we have multiple cases where this can happen.

• Using literal(123) we will always create a LongLiteral, since Python has no distinction between an Integer and a Long.
• Also, when we create a bytes literal literal(b'\00') for the FixedLiteral:

  >>> from pyiceberg.expressions.literals import literal
  >>> literal(b'\0x00')
  BinaryLiteral(b'\x00x00')

  We might also need to scale this when we bind it to a FixedLiteral field (binary field, with a fixed length). Upon binding, we should convert the type to the one in the schema, and check if the value is still within bounds (doesn't overflow an int in the case of Long to Integer, or the binary field is greater than the size of the FixedLength).

[hendrikmakait]

@rdblue: My main point is that this change affects debugging, which __repr__ is intended for (https://docs.python.org/3/reference/datamodel.html#object.__repr__). This will make it more involved to figure out what exact object one is dealing with and to see whether code, e.g., conversions, worked as intended from just glancing at debugger output. For example, the conversion LongLiteral(1234) -> FloatLiteral(1234.0) will become a literal(1234) -> literal(1234.0). While we are technically able to see that some conversion is happening, we have to spot the change in .0 and we still don't know whether it's a FloatLiteral or DoubleLiteral.

[Fokko]

Closing this one since there is no real consensus.