feat: add advanced extension field to DdlRel, WriteRel, and UpdateRel

[jcamachor]

No description provided.

[jacques-n]

What are general thoughts on creating field here versus the one already available in RelCommon? RelCommon is kind of like an abstract superclass (if you will) except that protobuf makes you invert the construction.

[jcamachor]

Thanks, @jacques-n . I was trying to follow the pattern used for other operators. My assumption was that both fields are included because it allows to have an extension implementation common to all operators (the one under RelCommon) as well as an extension specific to a given operator.

[jacques-n]

Thanks, @jacques-n . I was trying to follow the pattern used for other operators....

Fair. I think we never really figured it out and it shows.

[jacques-n]

The one problem I struggle with is the in relation location means these are by default semantic changing so a system receiving something at this position must understand or fail the query.

The benefit of using inside hint of relcommon is that an engine can ignore if it likes.

[jcamachor]

in relation location means these are by default semantic changing

Isn't it the case that whether these extensions are semantic changing by default depends on whether we use the optimization field or the enhancement field within the AdvancedExtension to define the extension?

[jacques-n]

in relation location means these are by default semantic changing

Isn't it the case that whether these extensions are semantic changing by default depends on whether we use the optimization field or the enhancement field within the AdvancedExtension to define the extension?

lol. yes.

[vbarua]

Overall looks reasonable to me. This brings these inline with all the other rels which also have AdvancedExtensions available.

I did leave some comments about the field number we're using. I don't know if it makes sense to reserve fields as opposed to just using the next available one.

[jcamachor]

I don't know if it makes sense to reserve fields as opposed to just using the next available one.

Thanks, @vbarua . For this, I used the same convention that I observed in extensions defined in other ops. Maybe let's reach consensus with other committers before I change it? @jacques-n , any preferences?

[jcamachor]

@vbarua , @jacques-n , how do you think we should proceed here? Thanks

[jcamachor]

@vbarua , @jacques-n , how do you think we should proceed here? Thanks

I have updated the field numbers.

[jacques-n]

I agree with @vbarua that we should just use next available. I didn't notice that on my review. It looks like we're still using not-next?

[jcamachor]

In the most recent commit, I changed it to use next available value.

[jcamachor]

@jacques-n , @vbarua , all feedback has been addressed, could we merge this change? (I believe current check failure is unrelated to this change but I'm not super familiar with it.) Thanks