GH-836: Added support of ExtensionType for ComplexCopier

[xxlaykxx]

What's Changed

Updated ComplexCopier to support ExtensionType - it contains two copy methods

public static void copy(FieldReader input, FieldWriter output) //for not breaking  existing logic

public static void copy(FieldReader input, FieldWriter output, ExtensionTypeWriterFactory extensionTypeWriterFactory)

Also updated ComplexCopier tests.
Closes #836.

[xxlaykxx]

@lidavidm Could you plz take a look?

[lidavidm]

Why do we need the factory, vs. being able to copy the "storage type" directly?

[lidavidm]

I don't think we can ever support extension without the factory right?

Suggested change

	throw new UnsupportedOperationException(
	"EXTENSIONTYPE are not supported yet. Please provide an ExtensionTypeWriterFactory." );
	throw new IllegalArgumentException(
	"Must provide ExtensionTypeWriterFactory" );

[xxlaykxx]

yep, we need a factory to determine writer impl for extension type.

[lidavidm]

Should these be abstract methods instead?

[xxlaykxx]

No, because this method is used only with complex type vectors and they have implementation; for non-complex, it's not supported, and this behavior is covered here

[xxlaykxx]

Why do we need the factory, vs. being able to copy the "storage type" directly?

If we use just the storage type, original (extension) type/vector will be missed

[jhrotko]

@lidavidm Would you mind taking another look at this PR when you have a chance? We would really appreciate your feedback. Thanks!

[lidavidm]

I guess my question is then, how does this work if we have more than one extension type (e.g. a struct vector with two extension type fields), given that everything is based around having 0 or 1 ExtensionTypeWriterFactorys?

[xxlaykxx]

I guess my question is then, how does this work if we have more than one extension type (e.g. a struct vector with two extension type fields), given that everything is based around having 0 or 1 ExtensionTypeWriterFactorys?

Because ExtensionTypeWriterFactory.getWriterImpl will return a writer based on vector type, so if will be several extension types - getWriterImpl should just handle this something like

if (extensionTypeVector instanceof TimeStampWithPrecisionVector) {
      return new TimeStampWithPrecisionWriterImpl(
          (TimeStampWithPrecisionVector) extensionTypeVector);
} else if (extensionTypeVector instanceof UuidVector) {
      return new UuidWriterImpl((UuidVector) extensionTypeVector);
}

[lidavidm]

Works for me.

Incidentally, ExtensionTypeWriterFactory is type-parameterized but it seems we never actually use it that way - can you file an issue to drop that and have getWriter just return FieldWriter?

[xxlaykxx]

Works for me.

Incidentally, ExtensionTypeWriterFactory is type-parameterized but it seems we never actually use it that way - can you file an issue to drop that and have getWriter just return FieldWriter?

#861

[lidavidm]

There's still pre-commit failures - might be worth running the formatter locally? mvn spotless:apply

[xxlaykxx]

@lidavidm could you re-check plz?