Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHOENIX-5580 Wrong values seen when updating a view for a table that … #768

Closed
wants to merge 3 commits into from

Conversation

sguggilam
Copy link

…has an index

@@ -2896,6 +2916,49 @@ public int getIndexDataColumnCount() {
return indexDataColumnCount_;
}

// optional string parentTableType = 24;
public static final int PARENTTABLETYPE_FIELD_NUMBER = 24;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: PARENT_TABLE_TYPE_FIELD_NUMBER

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is auto generated file

* <code>optional string parentTableType = 24;</code>
*/
public com.google.protobuf.ByteString
getParentTableTypeBytes() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can fit in one line, any reason to break to multiple? applies to other places as well

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is auto generated file

/**
* <code>optional string parentTableType = 24;</code>
*/
public Builder setParentTableType(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent looks little off in this function

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is auto generated file

@@ -1887,6 +1899,11 @@ public boolean isImmutableRows() {
return immutableRows;
}

public boolean isTableIndex() {
if (parentTableType == null) return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

braces in if block

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -1887,6 +1899,11 @@ public boolean isImmutableRows() {
return immutableRows;
}

public boolean isTableIndex() {
Copy link
Contributor

@swaroopak swaroopak Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't come across anything as a table index and might confuse while reading as global index comes with its own table. How about isViewIndex() and use this with a negation at line#1667?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, made the change and pushed in latest commit

@@ -1887,6 +1899,13 @@ public boolean isImmutableRows() {
return immutableRows;
}

public boolean isViewIndex() {
if (parentTableType == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sguggilam - In the upgrade scenario where the clients don't have the new proto and hence parentTableType isn't defined, will this still work OK to have isViewIndex incorrectly return true?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes @gjacoby126 , the logic that is added in the change makes sure that the behavior is unchanged for the clients that doesn't pass the parentTableType field (the logic uses a OR condition with !isViewIndex() )

@stoty
Copy link
Contributor

stoty commented Aug 1, 2023

Already merged.

@stoty stoty closed this Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants