PHOENIX-374: Enable access to dynamic columns in * or cf.* selection #428

ChinmaySKulkarni · 2019-01-15T18:02:46Z

Using 1 cell per dyn col for metadata storage. Upsert path done. Select path done. Serializing
dynamic column PColumn list and sending back in result Cell list done. Combined with
PhoenixResultSet rowprojector so that dynamic columns can directly be
used with the help of PhoenixResultSetMetaData. Supporting column family wildcards as well. Added tests.
Design Doc: PHOENIX-374 Design Doc

ChinmaySKulkarni · 2019-01-22T06:07:10Z

Please review @twdsilva, @gjacoby126, @vincentpoon @kadirozde if you have some time.

twdsilva

@ChinmaySKulkarni Looks very good so far, I added some comments.

twdsilva · 2019-01-23T17:54:21Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/DynamicColumnWildcardIT.java

+ * turned on
+ */
+@RunWith(Parameterized.class)
+public class DynamicColumnWildcardIT extends BaseTest {


Nice test coverage!

twdsilva · 2019-01-23T17:59:48Z

phoenix-core/src/main/java/org/apache/phoenix/compile/ProjectionCompiler.java

-        return new RowProjector(projectedColumns, Math.max(estimatedKeySize, estimatedByteSize), isProjectEmptyKeyValue, resolver.hasUDFs(), isWildcard);
+        return new RowProjector(projectedColumns, Math.max(estimatedKeySize, estimatedByteSize),
+                isProjectEmptyKeyValue, resolver.hasUDFs(), isWildcard,
+                isWildcard || !projectedFamilies.isEmpty());


do you need to include the isWildcard boolean check?

This should actually be the wildcardIncludesDynamicCols check. Will change it.

twdsilva · 2019-01-23T19:12:19Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/DynamicColumnWildcard.java

+ * QueryServices.WILDCARD_QUERY_DYNAMIC_COLS_ATTRIB
+ * See PHOENIX-374
+ */
+public class DynamicColumnWildcard implements RegionObserver, RegionCoprocessor {


Can you just use ScanRegionObserver instead of creating a new coprocessor?

@twdsilva One problem with this approach is that now the preBatchMutate will be called in every case, irrespective of the client-side config to enable dynamic column data in wildcard queries. With the current implementation, even if this config is off (and thus there are no attributes set on any of the mutations), we will still have to:

In the worst case (all Puts), iterate over (Number of mutations in miniBatchOp) * (Number of column families in each mutation), though ultimately it will be a no-op.

In the best case (all Deletes), iterate over (Number of mutations in miniBatchOp), though ultimately it will be a no-op.

At best, I can set an additional attribute on the Put if we don't want to store any dynamic column metadata and make sure that if the config is off, we will always only iterate over (Number of mutations in miniBatchOp) and do no-op. There is still the additional iterations over all the mutations though. Is there a way to pass client-side configs to the server-side via the miniBatchOp? If so, I can completely short-circuit this logic in case the config is off.

Went ahead with with the additional attribute to avoid unnecessary processing in case the config is off or there are no dynamic columns. Please take a look now @twdsilva

twdsilva · 2019-01-23T19:14:49Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/DynamicColumnWildcard.java

+    // Scan attribute that is set in case we want to project dynamic columns
+    public static final String WILDCARD_SCAN_INCLUDES_DYNAMIC_COLUMNS =
+            "_WildcardScanIncludesDynCols";
+    public static final byte[] DYN_COLS_METADATA_CELL_QUALIFIER = Bytes.toBytes("#Dyn_");


Can you change this to something shorter to save space maybe "D#" ?

twdsilva · 2019-01-23T19:16:59Z

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java

+
+            boolean wildcardIncludesDynamicCols = props.getBoolean(
+                    WILDCARD_QUERY_DYNAMIC_COLS_ATTRIB, DEFAULT_WILDCARD_QUERY_DYNAMIC_COLS_ATTRIB);
+            // We register the coprocessor for dynamic columns to be exposed when issuing wildcard


We need need to always load this coprocessor, since you could have a mix of clients that have this config turned on/off.

twdsilva · 2019-01-23T19:21:50Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/DynamicColumnWildcard.java

+                            .getDynamicColumnsList();
+                    for (PTableProtos.PColumn dynColProto : dynColsInThisFam) {
+                        // Add a column for this dynamic column to the metadata Put operation
+                        ((Put)putDynColMetaData.get(0)).addColumn(fam,


nit: can you create a local variable for the put and then call miniBatchOp.addOperationsFromCP if its not null

twdsilva · 2019-01-23T19:24:08Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/DynamicColumnWildcard.java

+                putDynColMetaData.add(new Put(m.getRow()));
+                NavigableMap<byte[], List<Cell>> famCellMap = m.getFamilyCellMap();
+                for (byte[] fam : famCellMap.keySet()) {
+                    byte[] serializedDynColsList = m.getAttribute(Bytes.toString(fam));


the attribute that store the serialized dynamic pcolumns is named the same as the family name?

Yes. The Put's attribute key is the column family name and the value is the serialized list of dynamic columns in that column family

ChinmaySKulkarni · 2019-02-04T18:24:49Z

@twdsilva Addressed review comments. Please review. Once it looks good, I'll squash and rebase with master. Thanks!

twdsilva · 2019-02-04T22:40:43Z

@ChinmaySKulkarni +1, I just had some minor feedback that you can fix before committing.

gokceni · 2019-02-04T23:05:49Z

phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java


-        public PRowImpl(KeyValueBuilder kvBuilder, ImmutableBytesWritable key, long ts, Integer bucketNum, boolean hasOnDupKey) {
+        PRowImpl(KeyValueBuilder kvBuilder, ImmutableBytesWritable key, long ts, Integer bucketNum, boolean hasOnDupKey) {


Question: Why the change from public to protector?

@gokceni This method does not require public scope hence I changed it to package-private

twdsilva · 2019-02-04T22:36:26Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/ScanRegionObserver.java

+            Mutation m = miniBatchOp.getOperation(i);
+            // There is at max 1 extra Put (for dynamic column shadow cells) per original Put
+            Put dynColShadowCellsPut = null;
+            if (m instanceof Put && !Bytes.equals(m.getAttribute(


Older clients won't have this attribute set on the mutation and it will enter this loop, IMO its better to invert this flag.

Good catch!

twdsilva · 2019-02-04T22:38:01Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/ScanRegionObserver.java

+        ByteArrayOutputStream qual = new ByteArrayOutputStream();
+        qual.write(DYN_COLS_METADATA_CELL_QUALIFIER);
+        qual.write(dynCol.getColumnQualifierBytes());
+        if (LOG.isDebugEnabled()) {


This might end up being a lot of logs, change it to the TRACE level.

twdsilva · 2019-02-04T22:39:24Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/ScanRegionObserver.java

+            Put dynColShadowCellsPut = null;
+            if (m instanceof Put && !Bytes.equals(m.getAttribute(
+                    NO_DYNAMIC_COLUMN_METADATA_STORED_FOR_MUTATION), TRUE_BYTES)) {
+                NavigableMap<byte[], List<Cell>> famCellMap = m.getFamilyCellMap();


Can you add a debug log line here?

ChinmaySKulkarni · 2019-02-07T23:22:54Z

Committed to master, 4.x-HBase-1.3 and 4.x-HBase-1.4. Thanks for the review @twdsilva and @gokceni !

ChinmaySKulkarni force-pushed the PHOENIX-374 branch 11 times, most recently from b6c2a53 to a9951fa Compare January 22, 2019 06:01

ChinmaySKulkarni force-pushed the PHOENIX-374 branch from a9951fa to bbb31be Compare January 22, 2019 06:14

twdsilva reviewed Jan 23, 2019

View reviewed changes

gokceni reviewed Feb 4, 2019

View reviewed changes

twdsilva reviewed Feb 4, 2019

View reviewed changes

ChinmaySKulkarni force-pushed the PHOENIX-374 branch 2 times, most recently from 27a5f0d to 176b059 Compare February 5, 2019 02:26

PHOENIX-374: Enable access to dynamic columns in * or cf.* selection

376775e

ChinmaySKulkarni force-pushed the PHOENIX-374 branch from 176b059 to 376775e Compare February 5, 2019 19:53

ChinmaySKulkarni closed this Feb 7, 2019

ChinmaySKulkarni deleted the PHOENIX-374 branch February 7, 2019 23:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-374: Enable access to dynamic columns in * or cf.* selection #428

PHOENIX-374: Enable access to dynamic columns in * or cf.* selection #428

ChinmaySKulkarni commented Jan 15, 2019 •

edited

Loading

ChinmaySKulkarni commented Jan 22, 2019

twdsilva left a comment

twdsilva Jan 23, 2019

ChinmaySKulkarni Jan 29, 2019

twdsilva Jan 23, 2019

ChinmaySKulkarni Jan 29, 2019

twdsilva Jan 23, 2019

ChinmaySKulkarni Jan 29, 2019 •

edited

Loading

ChinmaySKulkarni Feb 3, 2019

twdsilva Jan 23, 2019

twdsilva Jan 23, 2019

twdsilva Jan 23, 2019

twdsilva Jan 23, 2019

ChinmaySKulkarni Jan 29, 2019

ChinmaySKulkarni commented Feb 4, 2019 •

edited

Loading

twdsilva commented Feb 4, 2019

gokceni Feb 4, 2019

ChinmaySKulkarni Feb 4, 2019

twdsilva Feb 4, 2019

ChinmaySKulkarni Feb 5, 2019

twdsilva Feb 4, 2019

twdsilva Feb 4, 2019

ChinmaySKulkarni commented Feb 7, 2019


		public PRowImpl(KeyValueBuilder kvBuilder, ImmutableBytesWritable key, long ts, Integer bucketNum, boolean hasOnDupKey) {
		PRowImpl(KeyValueBuilder kvBuilder, ImmutableBytesWritable key, long ts, Integer bucketNum, boolean hasOnDupKey) {

PHOENIX-374: Enable access to dynamic columns in * or cf.* selection #428

PHOENIX-374: Enable access to dynamic columns in * or cf.* selection #428

Conversation

ChinmaySKulkarni commented Jan 15, 2019 • edited Loading

ChinmaySKulkarni commented Jan 22, 2019

twdsilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChinmaySKulkarni Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChinmaySKulkarni commented Feb 4, 2019 • edited Loading

twdsilva commented Feb 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChinmaySKulkarni commented Feb 7, 2019

ChinmaySKulkarni commented Jan 15, 2019 •

edited

Loading

ChinmaySKulkarni Jan 29, 2019 •

edited

Loading

ChinmaySKulkarni commented Feb 4, 2019 •

edited

Loading