fix: i16 i8 in literal_type

[zhejiangxiaomai]

• Fix problem of i16 i8 type in literal_type.
  In our project, when use substraitLit.i16() to a SMALLINT(2-Byte type), we get INTEGER(4-Byte type).

[jacques-n]

This is a breaking change so at a minimum, we'd need to fix that.

I'm not sure this is a critical change to make. Thoughts @cpcloud , @jvanstraten, @westonpace ?

[jvanstraten]

Well... I'm not going to say no to fixing something that's unnecessarily inconsistent. I'm not sure how much it'd actually break either; the values this makes illegal in protobuf were already semantically illegal, and aren't all ints stored the same way in binary protobuf? The worst I could see this breaking is consumer/producer code in particularly type-safe programming languages... but I can make the same argument against adding a oneof entry, which is a breaking change in Rust due to match statements needing to list all options exhaustively.

This is the kind of minor cleanup I'd personally prefer to batch up vs making one breaking change at a time, though. IIRC there are a bunch of cases relating to random use of signed vs unsigned integers for inherently unsigned things, for example. But I won't die on that hill.

+0.9 I guess?

[westonpace]

Probably the most significant break would be that any binary plans generated by an older version would be unreadable by a newer version (assuming they had some of these literals). That being said, we should probably recommend against relying on stored binary Substrait plans until 1.0. I'm +1 on the change.

I don't see much benefit in bunching. If users want to bunch the changes then they can just hold off on pulling the update (though admittedly, they wouldn't be able to cherry pick new features without pulling this change in) but I feel like managing the bunching would be tedious (but if someone is volunteering to do it then that's fine with me).

[jvanstraten]

Okay, hang on. I just double-checked that all int and uint types have the same encoding in protobuf. Only sint types would decode incorrectly. However... protobuf simply doesn't support int8 and int16, the smallest type is int32. CI confirms that the proposed change simply doesn't compile. So I'm pretty sure this PR is invalid.

@westonpace, the way I see it, bunching is about compatibility between different projects, not so much about how slow or fast people want to track the changes we make. People could also just point to git hashes rather than versions if they want to track incrementally, after all. The problem I see is that if we do lots of different incompatible version releases, the odds of different projects coincidentally supporting the same one and actually working together is greatly diminished.

[westonpace]

@westonpace, the way I see it, bunching is about compatibility between different projects, not so much about how slow or fast people want to track the changes we make. People could also just point to git hashes rather than versions if they want to track incrementally, after all. The problem I see is that if we do lots of different incompatible version releases, the odds of different projects coincidentally supporting the same one and actually working together is greatly diminished.

That makes sense. Do you think a pre-requisite for bunching would be to have a scheme devised for putting version numbers in plans?

[zhejiangxiaomai]

Ok, I see. I will use static_cast to make a type conversion to int16_t. when we use substraitLit.i16().

[jvanstraten]

Do you think a pre-requisite for bunching would be to have a scheme devised for putting version numbers in plans?

I don't think it's a pre-requisite per se, but I'd like to see that added sooner than later. In fact: #347. It's not a breaking change though.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[westonpace]

Can this be closed or is there still an action to take?