Re: [1.13 - Release Blocker] - DRILL-6216: Metadata mismatch when connecting to a Drill 1.12.0 with a Drill-1.13.0-SNAPSHOT driver

[Parth Chandra]

Paul , thanks for confirming that it is safe to revert.


On Wed, Mar 7, 2018 at 11:52 AM, Paul Rogers 
wrote:

> Hi Sorabh,
> Thanks for tracking this one down. Our unit tests did not uncover this
> issue when I did the original PR, unfortunately. The name change was done
> to be consistent with other places where we use special names and, as I
> recall, help with certain tasks.
> Clearly, however, if the client depends on the name, we cannot change the
> name without breaking the client. It is safe to simply revert the change
> and rerun the tests.
> (Having an API version number would have been handy here: we could detect
> the older client. But Drill, like most Hadoop components, has no API
> versioning mechanism, alas.)
> To prevent future issues, let's also change that bit of client code: there
> is no reason to assert the name; the structural relationship is all that
> should matter.
> Thanks,
> - Paul
>
>
>
> On Tuesday, March 6, 2018, 8:02:05 PM PST, Sorabh Hamirwasia <
> shamirwa...@mapr.com> wrote:
>
>  Hi All,
>
> The root cause for DRILL-6216 is due to a recent change made as part of
> https://issues.apache.org/jira/browse/DRILL-6049. With this PR a default
> field name for values ValueVector inside any NullableValueVector was
> introduced which is $values$ [1]. Before this PR the values ValueVector
> field name was same as the field name of actual NullableValueVector holding
> it [2]. In the load method of certain ValueVectors like BigIntVector there
> is an equality check for the ValueVector field name and metadata.name_part
> name [3].
>
> In setup where Drillbit and DrillClient are running in different version
> (between 1.12 and 1.13) the equality check in load method will fail. For
> example: When server is running 1.13 and client is on 1.12, in that case
> the record batch from server side will come with NullableValueVector (NV1
> with field name as num_val) but with it's values ValueVector field name as
> $values$. When on client side corresponding NullableValueVector (NV2) is
> created it will use the actual field name (num_val) for values ValueVector.
> After calling load on received NullableValueVector NV2 with metadata
> information from server that internally alls load on values ValueVector and
> the check fails since ($values$ != num_val).
>
>
> Since the change is in template of ValueVector, to fix this issue both
> client and server needs to identify their respective version on which they
> are running and choose the field name for values ValueVector
> correspondingly. Given DRILL-6049 touches huge sets of files I am also not
> sure what are the other impacts with it. It would be great to discuss on
> how we should proceed with this issue before making any further change.
>
>
> [1]: https://github.com/apache/drill/blob/master/exec/vector/
> src/main/java/org/apache/drill/exec/vector/ValueVector.java#L92
>
> https://github.com/apache/drill/blob/master/exec/vector/
> src/main/codegen/templates/NullableValueVectors.java#L82
>
> [2]:https://github.com/apache/drill/blob/1.12.0/exec/vector/
> src/main/codegen/templates/NullableValueVectors.java#L70
>
> [3]: https://github.com/apache/drill/blob/1.12.0/exec/vector/
> src/main/codegen/templates/FixedValueVectors.java#L238
>
>
> Thanks,
>
> Sorabh
>
>

Re: [1.13 - Release Blocker] - DRILL-6216: Metadata mismatch when connecting to a Drill 1.12.0 with a Drill-1.13.0-SNAPSHOT driver

[Paul Rogers]

Hi Sorabh,
Thanks for tracking this one down. Our unit tests did not uncover this issue 
when I did the original PR, unfortunately. The name change was done to be 
consistent with other places where we use special names and, as I recall, help 
with certain tasks.
Clearly, however, if the client depends on the name, we cannot change the name 
without breaking the client. It is safe to simply revert the change and rerun 
the tests.
(Having an API version number would have been handy here: we could detect the 
older client. But Drill, like most Hadoop components, has no API versioning 
mechanism, alas.)
To prevent future issues, let's also change that bit of client code: there is 
no reason to assert the name; the structural relationship is all that should 
matter.
Thanks,
- Paul

 

On Tuesday, March 6, 2018, 8:02:05 PM PST, Sorabh Hamirwasia 
 wrote:  
 
 Hi All,

The root cause for DRILL-6216 is due to a recent change made as part of 
https://issues.apache.org/jira/browse/DRILL-6049. With this PR a default field 
name for values ValueVector inside any NullableValueVector was introduced which 
is $values$ [1]. Before this PR the values ValueVector field name was same as 
the field name of actual NullableValueVector holding it [2]. In the load method 
of certain ValueVectors like BigIntVector there is an equality check for the 
ValueVector field name and metadata.name_part name [3].

In setup where Drillbit and DrillClient are running in different version 
(between 1.12 and 1.13) the equality check in load method will fail. For 
example: When server is running 1.13 and client is on 1.12, in that case the 
record batch from server side will come with NullableValueVector (NV1 with 
field name as num_val) but with it's values ValueVector field name as $values$. 
When on client side corresponding NullableValueVector (NV2) is created it will 
use the actual field name (num_val) for values ValueVector. After calling load 
on received NullableValueVector NV2 with metadata information from server that 
internally alls load on values ValueVector and the check fails since ($values$ 
!= num_val).


Since the change is in template of ValueVector, to fix this issue both client 
and server needs to identify their respective version on which they are running 
and choose the field name for values ValueVector correspondingly. Given 
DRILL-6049 touches huge sets of files I am also not sure what are the other 
impacts with it. It would be great to discuss on how we should proceed with 
this issue before making any further change.


[1]: 
https://github.com/apache/drill/blob/master/exec/vector/src/main/java/org/apache/drill/exec/vector/ValueVector.java#L92

https://github.com/apache/drill/blob/master/exec/vector/src/main/codegen/templates/NullableValueVectors.java#L82

[2]:https://github.com/apache/drill/blob/1.12.0/exec/vector/src/main/codegen/templates/NullableValueVectors.java#L70

[3]: 
https://github.com/apache/drill/blob/1.12.0/exec/vector/src/main/codegen/templates/FixedValueVectors.java#L238


Thanks,

Sorabh
  

Re: [1.13 - Release Blocker] - DRILL-6216: Metadata mismatch when connecting to a Drill 1.12.0 with a Drill-1.13.0-SNAPSHOT driver

[Parth Chandra]

+1 on reverting this if it doesn't break things.

On Wed, Mar 7, 2018 at 11:11 AM, Aman Sinha  wrote:

> Thanks Sorabh, for the analysis and the pointers to the relevant code.
> Doing the version check check for client and server would be the right
> thing to do but probably too disruptive at this stage of the release.  We
> should do it for the next release.
> For 1.13, could the change of the $values$ be reverted to the actual field
> name of the ValueVector ?  Since the DRILL-6049 was a 'hygiene' change, I
> would think that doing this revert should not break anything...but I am not
> fully sure of this.
>
> -Aman
>
> On Tue, Mar 6, 2018 at 8:01 PM, Sorabh Hamirwasia 
> wrote:
>
>> Hi All,
>>
>> The root cause for DRILL-6216 is due to a recent change made as part of
>> https://issues.apache.org/jira/browse/DRILL-6049. With this PR a default
>> field name for values ValueVector inside any NullableValueVector was
>> introduced which is $values$ [1]. Before this PR the values ValueVector
>> field name was same as the field name of actual NullableValueVector holding
>> it [2]. In the load method of certain ValueVectors like BigIntVector there
>> is an equality check for the ValueVector field name and metadata.name_part
>> name [3].
>>
>> In setup where Drillbit and DrillClient are running in different version
>> (between 1.12 and 1.13) the equality check in load method will fail. For
>> example: When server is running 1.13 and client is on 1.12, in that case
>> the record batch from server side will come with NullableValueVector (NV1
>> with field name as num_val) but with it's values ValueVector field name as
>> $values$. When on client side corresponding NullableValueVector (NV2) is
>> created it will use the actual field name (num_val) for values ValueVector.
>> After calling load on received NullableValueVector NV2 with metadata
>> information from server that internally alls load on values ValueVector and
>> the check fails since ($values$ != num_val).
>>
>>
>> Since the change is in template of ValueVector, to fix this issue both
>> client and server needs to identify their respective version on which they
>> are running and choose the field name for values ValueVector
>> correspondingly. Given DRILL-6049 touches huge sets of files I am also not
>> sure what are the other impacts with it. It would be great to discuss on
>> how we should proceed with this issue before making any further change.
>>
>>
>> [1]: https://github.com/apache/drill/blob/master/exec/vector/src/
>> main/java/org/apache/drill/exec/vector/ValueVector.java#L92
>>
>> https://github.com/apache/drill/blob/master/exec/vector/src/
>> main/codegen/templates/NullableValueVectors.java#L82
>>
>> [2]:https://github.com/apache/drill/blob/1.12.0/exec/vector/
>> src/main/codegen/templates/NullableValueVectors.java#L70
>>
>> [3]: https://github.com/apache/drill/blob/1.12.0/exec/vector/src/
>> main/codegen/templates/FixedValueVectors.java#L238
>>
>>
>> Thanks,
>>
>> Sorabh
>>
>>
>

Re: [1.13 - Release Blocker] - DRILL-6216: Metadata mismatch when connecting to a Drill 1.12.0 with a Drill-1.13.0-SNAPSHOT driver

[Aman Sinha]

Thanks Sorabh, for the analysis and the pointers to the relevant code.
Doing the version check check for client and server would be the right
thing to do but probably too disruptive at this stage of the release.  We
should do it for the next release.
For 1.13, could the change of the $values$ be reverted to the actual field
name of the ValueVector ?  Since the DRILL-6049 was a 'hygiene' change, I
would think that doing this revert should not break anything...but I am not
fully sure of this.

-Aman

On Tue, Mar 6, 2018 at 8:01 PM, Sorabh Hamirwasia 
wrote:

> Hi All,
>
> The root cause for DRILL-6216 is due to a recent change made as part of
> https://issues.apache.org/jira/browse/DRILL-6049. With this PR a default
> field name for values ValueVector inside any NullableValueVector was
> introduced which is $values$ [1]. Before this PR the values ValueVector
> field name was same as the field name of actual NullableValueVector holding
> it [2]. In the load method of certain ValueVectors like BigIntVector there
> is an equality check for the ValueVector field name and metadata.name_part
> name [3].
>
> In setup where Drillbit and DrillClient are running in different version
> (between 1.12 and 1.13) the equality check in load method will fail. For
> example: When server is running 1.13 and client is on 1.12, in that case
> the record batch from server side will come with NullableValueVector (NV1
> with field name as num_val) but with it's values ValueVector field name as
> $values$. When on client side corresponding NullableValueVector (NV2) is
> created it will use the actual field name (num_val) for values ValueVector.
> After calling load on received NullableValueVector NV2 with metadata
> information from server that internally alls load on values ValueVector and
> the check fails since ($values$ != num_val).
>
>
> Since the change is in template of ValueVector, to fix this issue both
> client and server needs to identify their respective version on which they
> are running and choose the field name for values ValueVector
> correspondingly. Given DRILL-6049 touches huge sets of files I am also not
> sure what are the other impacts with it. It would be great to discuss on
> how we should proceed with this issue before making any further change.
>
>
> [1]: https://github.com/apache/drill/blob/master/exec/vector/
> src/main/java/org/apache/drill/exec/vector/ValueVector.java#L92
>
> https://github.com/apache/drill/blob/master/exec/vector/
> src/main/codegen/templates/NullableValueVectors.java#L82
>
> [2]:https://github.com/apache/drill/blob/1.12.0/exec/vector/
> src/main/codegen/templates/NullableValueVectors.java#L70
>
> [3]: https://github.com/apache/drill/blob/1.12.0/exec/vector/
> src/main/codegen/templates/FixedValueVectors.java#L238
>
>
> Thanks,
>
> Sorabh
>
>