ARROW-8948: [Java][Integration] enable duplicate field names integration tests

[rymurr]

This addresses the integration test error where Java cannot process duplicate field names.

This extends StructVector and VectorSchemaRoot to support duplicate field names.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-8948

[lidavidm]

I took an initial pass. I'm not super familiar with the internals here so I can't comment on the approach.

[lidavidm]

If I'm not mistaken - this means the "correct" or "compatible" behavior is opt-in via a JVM flag? Are these flags clearly documented somewhere, I think we have a few others?

[rymurr]

Hey @lidavidm, yes the 'compatible' behaviour is opt-in. This may be controversial but through a mix of opinionated, ignorant and lazy I don't understand the value of duplicate names in a struct. Given the scope of implementing such a change fully and the unintended consequences downstream I have opted to give the library user the option to be compatible with the c++ IPC or maintain backwards compatibility with Java. I am happy to hear what the community thinks, especially if this approach is seen as too heavy handed.

This flag wasn't document anywhere so I have added a note to the 'Java Properties' section in the Java README.md.

[lidavidm]

I think as long as it's easily controllable in code (so a single application can work with both behaviors) and well-documented, that should be OK. I dislike only having global flags, but that's not the case here. (And having global flags can be useful to tweak the behavior of an application that's otherwise agnostic to the default behavior.)

[lidavidm]

This does change the complexity - though presumably it's not an issue unless someone has tens of thousands of columns or is repeatedly fetching vectors in a tight loop.

[rymurr]

Agreed, this now matches with the Schema implementation. I think we should either:
i) deprecate the name based methods
ii) document that these methods are unsuitable for tight loops.
Any thoughts on which is more sensible?

[lidavidm]

This no longer extends the Map<K,V> interface - is that an acceptable break?

[lidavidm]

(Presumably this was for internal use only.)

[rymurr]

I felt it was acceptable to break from Map as it is internal and only used by Structs. I surmise that the intention was for this class to be a more generic utility and that need never materialised

[emkornfield]

we've been using the policy that java properties should also be settable via environment variable (I believe in some context environment variables are easier to set).

[rymurr]

done!

[emkornfield]

is there a reason no to change this to a Map<String, List>?

[rymurr]

I didn't want to lose the type information. This still allows you to address Field("x", int) compared to Field("x", long).

[emkornfield]

if you move the enum above this block, are you able to access the CONFLICT_REPLACE enum directly, to use it instead oa constant here?

[rymurr]

fixed

[nealrichardson]

@rymurr@lidavidm@emkornfield is this good to go now?

[rymurr]

Thanks @nealrichardson , I am happy if @lidavidm and @emkornfield are!

[lidavidm]

I am good with this.

[rymurr]

This addresses the integration test error where Java cannot process duplicate field names.

This extends StructVector and VectorSchemaRoot to support duplicate field names.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-8948

[lidavidm]

I took an initial pass. I'm not super familiar with the internals here so I can't comment on the approach.

[lidavidm]

If I'm not mistaken - this means the "correct" or "compatible" behavior is opt-in via a JVM flag? Are these flags clearly documented somewhere, I think we have a few others?

[rymurr]

Hey @lidavidm, yes the 'compatible' behaviour is opt-in. This may be controversial but through a mix of opinionated, ignorant and lazy I don't understand the value of duplicate names in a struct. Given the scope of implementing such a change fully and the unintended consequences downstream I have opted to give the library user the option to be compatible with the c++ IPC or maintain backwards compatibility with Java. I am happy to hear what the community thinks, especially if this approach is seen as too heavy handed.

This flag wasn't document anywhere so I have added a note to the 'Java Properties' section in the Java README.md.

[lidavidm]

I think as long as it's easily controllable in code (so a single application can work with both behaviors) and well-documented, that should be OK. I dislike only having global flags, but that's not the case here. (And having global flags can be useful to tweak the behavior of an application that's otherwise agnostic to the default behavior.)

[lidavidm]

This does change the complexity - though presumably it's not an issue unless someone has tens of thousands of columns or is repeatedly fetching vectors in a tight loop.

[rymurr]

Agreed, this now matches with the Schema implementation. I think we should either:
i) deprecate the name based methods
ii) document that these methods are unsuitable for tight loops.
Any thoughts on which is more sensible?

[lidavidm]

This no longer extends the Map<K,V> interface - is that an acceptable break?

[lidavidm]

(Presumably this was for internal use only.)

[rymurr]

I felt it was acceptable to break from Map as it is internal and only used by Structs. I surmise that the intention was for this class to be a more generic utility and that need never materialised

[emkornfield]

we've been using the policy that java properties should also be settable via environment variable (I believe in some context environment variables are easier to set).

[rymurr]

done!

[emkornfield]

is there a reason no to change this to a Map<String, List>?

[rymurr]

I didn't want to lose the type information. This still allows you to address Field("x", int) compared to Field("x", long).

[emkornfield]

if you move the enum above this block, are you able to access the CONFLICT_REPLACE enum directly, to use it instead oa constant here?

[rymurr]

fixed

[nealrichardson]

@rymurr@lidavidm@emkornfield is this good to go now?

[rymurr]

Thanks @nealrichardson , I am happy if @lidavidm and @emkornfield are!

[lidavidm]

I am good with this.

[rymurr]

This addresses the integration test error where Java cannot process duplicate field names.

This extends StructVector and VectorSchemaRoot to support duplicate field names.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-8948

[lidavidm]

I took an initial pass. I'm not super familiar with the internals here so I can't comment on the approach.

[lidavidm]

If I'm not mistaken - this means the "correct" or "compatible" behavior is opt-in via a JVM flag? Are these flags clearly documented somewhere, I think we have a few others?

[rymurr]

Hey @lidavidm, yes the 'compatible' behaviour is opt-in. This may be controversial but through a mix of opinionated, ignorant and lazy I don't understand the value of duplicate names in a struct. Given the scope of implementing such a change fully and the unintended consequences downstream I have opted to give the library user the option to be compatible with the c++ IPC or maintain backwards compatibility with Java. I am happy to hear what the community thinks, especially if this approach is seen as too heavy handed.

This flag wasn't document anywhere so I have added a note to the 'Java Properties' section in the Java README.md.

[lidavidm]

I think as long as it's easily controllable in code (so a single application can work with both behaviors) and well-documented, that should be OK. I dislike only having global flags, but that's not the case here. (And having global flags can be useful to tweak the behavior of an application that's otherwise agnostic to the default behavior.)

[lidavidm]

This does change the complexity - though presumably it's not an issue unless someone has tens of thousands of columns or is repeatedly fetching vectors in a tight loop.

[rymurr]

Agreed, this now matches with the Schema implementation. I think we should either:
i) deprecate the name based methods
ii) document that these methods are unsuitable for tight loops.
Any thoughts on which is more sensible?

[lidavidm]

This no longer extends the Map<K,V> interface - is that an acceptable break?

[lidavidm]

(Presumably this was for internal use only.)

[rymurr]

I felt it was acceptable to break from Map as it is internal and only used by Structs. I surmise that the intention was for this class to be a more generic utility and that need never materialised

[emkornfield]

we've been using the policy that java properties should also be settable via environment variable (I believe in some context environment variables are easier to set).

[rymurr]

done!

[emkornfield]

is there a reason no to change this to a Map<String, List>?

[rymurr]

I didn't want to lose the type information. This still allows you to address Field("x", int) compared to Field("x", long).

[emkornfield]

if you move the enum above this block, are you able to access the CONFLICT_REPLACE enum directly, to use it instead oa constant here?

[rymurr]

fixed

[nealrichardson]

@rymurr@lidavidm@emkornfield is this good to go now?

[rymurr]

Thanks @nealrichardson , I am happy if @lidavidm and @emkornfield are!

[lidavidm]

I am good with this.

[rymurr]

This addresses the integration test error where Java cannot process duplicate field names.

This extends StructVector and VectorSchemaRoot to support duplicate field names.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-8948

[lidavidm]

I took an initial pass. I'm not super familiar with the internals here so I can't comment on the approach.

[lidavidm]

If I'm not mistaken - this means the "correct" or "compatible" behavior is opt-in via a JVM flag? Are these flags clearly documented somewhere, I think we have a few others?

[rymurr]

Hey @lidavidm, yes the 'compatible' behaviour is opt-in. This may be controversial but through a mix of opinionated, ignorant and lazy I don't understand the value of duplicate names in a struct. Given the scope of implementing such a change fully and the unintended consequences downstream I have opted to give the library user the option to be compatible with the c++ IPC or maintain backwards compatibility with Java. I am happy to hear what the community thinks, especially if this approach is seen as too heavy handed.

This flag wasn't document anywhere so I have added a note to the 'Java Properties' section in the Java README.md.

[lidavidm]

I think as long as it's easily controllable in code (so a single application can work with both behaviors) and well-documented, that should be OK. I dislike only having global flags, but that's not the case here. (And having global flags can be useful to tweak the behavior of an application that's otherwise agnostic to the default behavior.)

[lidavidm]

This does change the complexity - though presumably it's not an issue unless someone has tens of thousands of columns or is repeatedly fetching vectors in a tight loop.

[rymurr]

Agreed, this now matches with the Schema implementation. I think we should either:
i) deprecate the name based methods
ii) document that these methods are unsuitable for tight loops.
Any thoughts on which is more sensible?

[lidavidm]

This no longer extends the Map<K,V> interface - is that an acceptable break?

[lidavidm]

(Presumably this was for internal use only.)

[rymurr]

I felt it was acceptable to break from Map as it is internal and only used by Structs. I surmise that the intention was for this class to be a more generic utility and that need never materialised

[emkornfield]

we've been using the policy that java properties should also be settable via environment variable (I believe in some context environment variables are easier to set).

[rymurr]

done!

[emkornfield]

is there a reason no to change this to a Map<String, List>?

[rymurr]

I didn't want to lose the type information. This still allows you to address Field("x", int) compared to Field("x", long).

[emkornfield]

if you move the enum above this block, are you able to access the CONFLICT_REPLACE enum directly, to use it instead oa constant here?

[rymurr]

fixed

[nealrichardson]

@rymurr@lidavidm@emkornfield is this good to go now?

[rymurr]

Thanks @nealrichardson , I am happy if @lidavidm and @emkornfield are!

[lidavidm]

I am good with this.

[rymurr]

This addresses the integration test error where Java cannot process duplicate field names.

This extends StructVector and VectorSchemaRoot to support duplicate field names.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-8948

[lidavidm]

I took an initial pass. I'm not super familiar with the internals here so I can't comment on the approach.

[lidavidm]

If I'm not mistaken - this means the "correct" or "compatible" behavior is opt-in via a JVM flag? Are these flags clearly documented somewhere, I think we have a few others?

[rymurr]

Hey @lidavidm, yes the 'compatible' behaviour is opt-in. This may be controversial but through a mix of opinionated, ignorant and lazy I don't understand the value of duplicate names in a struct. Given the scope of implementing such a change fully and the unintended consequences downstream I have opted to give the library user the option to be compatible with the c++ IPC or maintain backwards compatibility with Java. I am happy to hear what the community thinks, especially if this approach is seen as too heavy handed.

This flag wasn't document anywhere so I have added a note to the 'Java Properties' section in the Java README.md.

[lidavidm]

I think as long as it's easily controllable in code (so a single application can work with both behaviors) and well-documented, that should be OK. I dislike only having global flags, but that's not the case here. (And having global flags can be useful to tweak the behavior of an application that's otherwise agnostic to the default behavior.)

[lidavidm]

This does change the complexity - though presumably it's not an issue unless someone has tens of thousands of columns or is repeatedly fetching vectors in a tight loop.

[rymurr]

Agreed, this now matches with the Schema implementation. I think we should either:
i) deprecate the name based methods
ii) document that these methods are unsuitable for tight loops.
Any thoughts on which is more sensible?

[lidavidm]

This no longer extends the Map<K,V> interface - is that an acceptable break?

[lidavidm]

(Presumably this was for internal use only.)

[rymurr]

I felt it was acceptable to break from Map as it is internal and only used by Structs. I surmise that the intention was for this class to be a more generic utility and that need never materialised

[emkornfield]

we've been using the policy that java properties should also be settable via environment variable (I believe in some context environment variables are easier to set).

[rymurr]

done!

[emkornfield]

is there a reason no to change this to a Map<String, List>?

[rymurr]

I didn't want to lose the type information. This still allows you to address Field("x", int) compared to Field("x", long).

[emkornfield]

if you move the enum above this block, are you able to access the CONFLICT_REPLACE enum directly, to use it instead oa constant here?

[rymurr]

fixed

[nealrichardson]

@rymurr@lidavidm@emkornfield is this good to go now?

[rymurr]

Thanks @nealrichardson , I am happy if @lidavidm and @emkornfield are!

[lidavidm]

I am good with this.

[rymurr]

This addresses the integration test error where Java cannot process duplicate field names.

This extends StructVector and VectorSchemaRoot to support duplicate field names.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-8948

[lidavidm]

I took an initial pass. I'm not super familiar with the internals here so I can't comment on the approach.

[lidavidm]

If I'm not mistaken - this means the "correct" or "compatible" behavior is opt-in via a JVM flag? Are these flags clearly documented somewhere, I think we have a few others?

[rymurr]

Hey @lidavidm, yes the 'compatible' behaviour is opt-in. This may be controversial but through a mix of opinionated, ignorant and lazy I don't understand the value of duplicate names in a struct. Given the scope of implementing such a change fully and the unintended consequences downstream I have opted to give the library user the option to be compatible with the c++ IPC or maintain backwards compatibility with Java. I am happy to hear what the community thinks, especially if this approach is seen as too heavy handed.

This flag wasn't document anywhere so I have added a note to the 'Java Properties' section in the Java README.md.

[lidavidm]

I think as long as it's easily controllable in code (so a single application can work with both behaviors) and well-documented, that should be OK. I dislike only having global flags, but that's not the case here. (And having global flags can be useful to tweak the behavior of an application that's otherwise agnostic to the default behavior.)

[lidavidm]

This does change the complexity - though presumably it's not an issue unless someone has tens of thousands of columns or is repeatedly fetching vectors in a tight loop.

[rymurr]

Agreed, this now matches with the Schema implementation. I think we should either:
i) deprecate the name based methods
ii) document that these methods are unsuitable for tight loops.
Any thoughts on which is more sensible?

[lidavidm]

This no longer extends the Map<K,V> interface - is that an acceptable break?

[lidavidm]

(Presumably this was for internal use only.)

[rymurr]

I felt it was acceptable to break from Map as it is internal and only used by Structs. I surmise that the intention was for this class to be a more generic utility and that need never materialised

[emkornfield]

we've been using the policy that java properties should also be settable via environment variable (I believe in some context environment variables are easier to set).

[rymurr]

done!

[emkornfield]

is there a reason no to change this to a Map<String, List>?

[rymurr]

I didn't want to lose the type information. This still allows you to address Field("x", int) compared to Field("x", long).

[emkornfield]

if you move the enum above this block, are you able to access the CONFLICT_REPLACE enum directly, to use it instead oa constant here?

[rymurr]

fixed

[nealrichardson]

@rymurr@lidavidm@emkornfield is this good to go now?

[rymurr]

Thanks @nealrichardson , I am happy if @lidavidm and @emkornfield are!

[lidavidm]

I am good with this.

[rymurr]

This addresses the integration test error where Java cannot process duplicate field names.

This extends StructVector and VectorSchemaRoot to support duplicate field names.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-8948

[lidavidm]

I took an initial pass. I'm not super familiar with the internals here so I can't comment on the approach.

[lidavidm]

If I'm not mistaken - this means the "correct" or "compatible" behavior is opt-in via a JVM flag? Are these flags clearly documented somewhere, I think we have a few others?

[rymurr]

Hey @lidavidm, yes the 'compatible' behaviour is opt-in. This may be controversial but through a mix of opinionated, ignorant and lazy I don't understand the value of duplicate names in a struct. Given the scope of implementing such a change fully and the unintended consequences downstream I have opted to give the library user the option to be compatible with the c++ IPC or maintain backwards compatibility with Java. I am happy to hear what the community thinks, especially if this approach is seen as too heavy handed.

This flag wasn't document anywhere so I have added a note to the 'Java Properties' section in the Java README.md.

[lidavidm]

I think as long as it's easily controllable in code (so a single application can work with both behaviors) and well-documented, that should be OK. I dislike only having global flags, but that's not the case here. (And having global flags can be useful to tweak the behavior of an application that's otherwise agnostic to the default behavior.)

[lidavidm]

This does change the complexity - though presumably it's not an issue unless someone has tens of thousands of columns or is repeatedly fetching vectors in a tight loop.

[rymurr]

Agreed, this now matches with the Schema implementation. I think we should either:
i) deprecate the name based methods
ii) document that these methods are unsuitable for tight loops.
Any thoughts on which is more sensible?

[lidavidm]

This no longer extends the Map<K,V> interface - is that an acceptable break?

[lidavidm]

(Presumably this was for internal use only.)

[rymurr]

I felt it was acceptable to break from Map as it is internal and only used by Structs. I surmise that the intention was for this class to be a more generic utility and that need never materialised

[emkornfield]

we've been using the policy that java properties should also be settable via environment variable (I believe in some context environment variables are easier to set).

[rymurr]

done!

[emkornfield]

is there a reason no to change this to a Map<String, List>?

[rymurr]

I didn't want to lose the type information. This still allows you to address Field("x", int) compared to Field("x", long).

[emkornfield]

if you move the enum above this block, are you able to access the CONFLICT_REPLACE enum directly, to use it instead oa constant here?

[rymurr]

fixed

[nealrichardson]

@rymurr@lidavidm@emkornfield is this good to go now?

[rymurr]

Thanks @nealrichardson , I am happy if @lidavidm and @emkornfield are!

[lidavidm]

I am good with this.

[rymurr]

This addresses the integration test error where Java cannot process duplicate field names.

This extends StructVector and VectorSchemaRoot to support duplicate field names.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-8948

[lidavidm]

I took an initial pass. I'm not super familiar with the internals here so I can't comment on the approach.

[lidavidm]

If I'm not mistaken - this means the "correct" or "compatible" behavior is opt-in via a JVM flag? Are these flags clearly documented somewhere, I think we have a few others?

[rymurr]

Hey @lidavidm, yes the 'compatible' behaviour is opt-in. This may be controversial but through a mix of opinionated, ignorant and lazy I don't understand the value of duplicate names in a struct. Given the scope of implementing such a change fully and the unintended consequences downstream I have opted to give the library user the option to be compatible with the c++ IPC or maintain backwards compatibility with Java. I am happy to hear what the community thinks, especially if this approach is seen as too heavy handed.

This flag wasn't document anywhere so I have added a note to the 'Java Properties' section in the Java README.md.

[lidavidm]

I think as long as it's easily controllable in code (so a single application can work with both behaviors) and well-documented, that should be OK. I dislike only having global flags, but that's not the case here. (And having global flags can be useful to tweak the behavior of an application that's otherwise agnostic to the default behavior.)

[lidavidm]

This does change the complexity - though presumably it's not an issue unless someone has tens of thousands of columns or is repeatedly fetching vectors in a tight loop.

[rymurr]

Agreed, this now matches with the Schema implementation. I think we should either:
i) deprecate the name based methods
ii) document that these methods are unsuitable for tight loops.
Any thoughts on which is more sensible?

[lidavidm]

This no longer extends the Map<K,V> interface - is that an acceptable break?

[lidavidm]

(Presumably this was for internal use only.)

[rymurr]

I felt it was acceptable to break from Map as it is internal and only used by Structs. I surmise that the intention was for this class to be a more generic utility and that need never materialised

[emkornfield]

we've been using the policy that java properties should also be settable via environment variable (I believe in some context environment variables are easier to set).

[rymurr]

done!

[emkornfield]

is there a reason no to change this to a Map<String, List>?

[rymurr]

I didn't want to lose the type information. This still allows you to address Field("x", int) compared to Field("x", long).

[emkornfield]

if you move the enum above this block, are you able to access the CONFLICT_REPLACE enum directly, to use it instead oa constant here?

[rymurr]

fixed

[nealrichardson]

@rymurr@lidavidm@emkornfield is this good to go now?

[rymurr]

Thanks @nealrichardson , I am happy if @lidavidm and @emkornfield are!

[lidavidm]

I am good with this.