[
https://issues.apache.org/jira/browse/PHOENIX-6897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17720671#comment-17720671
]
ASF GitHub Bot commented on PHOENIX-6897:
-----------------------------------------
kadirozde commented on code in PR #1597:
URL: https://github.com/apache/phoenix/pull/1597#discussion_r1187894947
##########
phoenix-core/src/main/java/org/apache/phoenix/index/GlobalIndexChecker.java:
##########
@@ -202,7 +208,42 @@ private void init() throws IOException {
else {
pageSize = Long.MAX_VALUE;
}
+
+ Filter filter = scan.getFilter();
+ if (filter instanceof PagingFilter) {
Review Comment:
We may also need to wrap non paging filters. So the same logic should be
applied even when the scan filter is not an instance of PagingFilter.
##########
phoenix-core/src/main/java/org/apache/phoenix/filter/UnverifiedRowFilter.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.filter;
+
+import java.io.IOException;
+import java.util.List;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.filter.Filter;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hbase.thirdparty.com.google.common.base.Preconditions;
+import org.apache.phoenix.util.ScanUtil;
+
+import static org.apache.phoenix.query.QueryConstants.VERIFIED_BYTES;
+
+/**
+ * This filter overrides the behavior of delegate so that we do not jump to
+ * the next row if the row is unverified and doesn't match the filter since
+ * it is possible that a previous verified version of the same row could match
+ * the filter and thus should be included in the results.
+ * For tables using encoded columns, the empty column is the first column the
+ * filter processes, so we can check whether it is verified or not.
+ * If no encoding is used, the empty column is the last column to be processed
+ * by the filter, so we have to wait to determine whether the row is verified
+ * or not.
+ */
+public class UnverifiedRowFilter extends DelegateFilter {
+
+ private final byte[] emptyCF;
+ private final byte[] emptyCQ;
+ private boolean verified = false;
+ // save the code from delegate filter while waiting for the empty column
+ private ReturnCode recordedRetCode = null;
+
+ private void init() {
+ verified = false;
+ recordedRetCode = null;
+ }
+ public UnverifiedRowFilter(Filter delegate, byte[] emptyCF, byte[]
emptyCQ) {
+ super(delegate);
+ Preconditions.checkArgument(emptyCF != null,
+ "Column family must not be null");
+ Preconditions.checkArgument(emptyCQ != null,
+ "Column qualifier must not be null");
+ this.emptyCF = emptyCF;
+ this.emptyCQ = emptyCQ;
+ init();
+ }
+
+ @Override
+ public void reset() throws IOException {
+ init();
+ delegate.reset();
+ }
+
+ @Override
+ public ReturnCode filterKeyValue(Cell v) throws IOException {
+ return filterCell(v);
+ }
+
+ @Override
+ public ReturnCode filterCell(final Cell cell) throws IOException {
+ if (verified) {
+ // we have processed the empty column and found that it is verified
+ return delegate.filterCell(cell);
+ }
+
+ if (ScanUtil.isEmptyColumn(cell, emptyCF, emptyCQ)) {
+ verified = Bytes.compareTo(
+ cell.getValueArray(), cell.getValueOffset(),
cell.getValueLength(),
+ VERIFIED_BYTES, 0, VERIFIED_BYTES.length) == 0;
+ if (verified) {
+ // if we saved the return code while waiting for the empty
+ // column, use that code else call the delegate
+ return recordedRetCode != null ? recordedRetCode :
delegate.filterCell(cell);
+ } else {
+ // it is an unverified row, no need to look at more columns
+ ReturnCode ret = delegate.filterCell(cell);
+ // Optimization for Skip scan delegate filter to avoid
+ // unnecessary read repairs if the filter returns seek to next
+ return ret != ReturnCode.SEEK_NEXT_USING_HINT
+ ? ReturnCode.INCLUDE_AND_SEEK_NEXT_ROW : ret;
Review Comment:
This means if the return code from the delegate is SEEK_NEXT_USING_HINT,
this filter also returns SEEK_NEXT_USING_HINT. However, this will result in
skipping the unverified row. This will filter should always return
INCLUDE_AND_SEEK_NEXT_ROW without checking the delegate's return code.
> Filters on unverified index rows return wrong result
> ----------------------------------------------------
>
> Key: PHOENIX-6897
> URL: https://issues.apache.org/jira/browse/PHOENIX-6897
> Project: Phoenix
> Issue Type: Bug
> Affects Versions: 5.1.2
> Reporter: Yunbo Fan
> Assignee: Tanuj Khurana
> Priority: Major
>
> h4. Summary:
> Upsert include three phases, and if failed after phase1, unverified index
> rows will leave in the index table. This will cause wrong result when do
> aggregate queries.
> h4. Steps for reproduce
> 1. create table and index
> {code}
> create table students(id integer primary key, name varchar, status integer);
> create index students_name_index on students(name, id) include (status);
> {code}
> 2. upsert data using phoenix
> {code}
> upsert into students values(1, 'tom', 1);
> upsert into students values(2, 'jerry', 2);
> {code}
> 3. do phase1 by hbase shell, change status column value to '2' and verified
> column value to '2'
> {code}
> put 'STUDENTS_NAME_INDEX', "tom\x00\x80\x00\x00\x01", '0:0:STATUS',
> "\x80\x00\x00\x02"
> put 'STUDENTS_NAME_INDEX', "tom\x00\x80\x00\x00\x01", '0:_0', "\x02"
> {code}
> notice: hbase shell can't parse colon in column, like '0:0:STATUS', you may
> need comment the line in hbase/lib/ruby/hbase/table.rb, see
> https://issues.apache.org/jira/browse/HBASE-13788
> {code}
> # Returns family and (when has it) qualifier for a column name
> def parse_column_name(column)
> split =
> org.apache.hadoop.hbase.KeyValue.parseColumn(column.to_java_bytes)
> -> comment this line out #set_converter(split) if split.length > 1
> return split[0], (split.length > 1) ? split[1] : nil
> end
> {code}
> 4. do query without aggregate, the result is right
> {code}
> 0: jdbc:phoenix:> select status from students where name = 'tom';
> +--------+
> | STATUS |
> +--------+
> | 1 |
> +--------+
> {code}
> 5. do query with aggregate, get wrong result
> {code}
> 0: jdbc:phoenix:> select count(*) from students where name = 'tom' and status
> = 1;
> +----------+
> | COUNT(1) |
> +----------+
> | 0 |
> +----------+
> {code}
> 6. using NO_INDEX hint
> {code}
> 0: jdbc:phoenix:> select /*+ NO_INDEX */ count(*) from students where name =
> 'tom' and status = 1;
> +----------+
> | COUNT(1) |
> +----------+
> | 1 |
> +----------+
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
