You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/03/24 04:55:18 UTC
[GitHub] [incubator-pinot] jamesyfshao opened a new pull request #5175: add
pinot upsert features to pinot common
jamesyfshao opened a new pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175
This diff will create necessary pinot schema/constant value changes to support pinot upsert features.
The following will be changed:
1. add updateSchematic field to schema and table definition:
this value can be either "upsert" or "append". When it is "upsert" pinot server/broker will apply proper upsert logic to ensure that table is updating based on primary key. a few notes:
- I am planning to simplify this config to be a boolean value, but due to several of our tables are already using this config, I will need to spend some time to configure out a way to migrate this properly.
- Currently, both schema & table requires this config to ensure that when we create table/schema they will have easy way to check if they are for upsert and ensure proper checks are enforced.
2. add primary key config:
this value will point to one of the column in this table as the primary key of this upsert table. Only matter if the table is for upsert
3. add offset key config:
this value will point to one of the column in this table to store the offset of this table. Only matter if the table is for upsert
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on issue #5175: add pinot
upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on issue #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-604111226
> > also [google doc design doc](https://www.google.com/url?sa=t&rct=j&esrc=s&source=appssearch&uact=8&cd=0&cad=rja&q&sig2=4ypfJoFtqjSdua_7dziWCQ&ved=0ahUKEwjVhpqIybToAhXDHWIKHWFcBVs4ABABKAAwAA&url=https://drive.google.com/a/uber.com/open?id%3D1SFFir7ByxCff-aVYxQeTHpNhPXeP5q7P4g_6O2iNGgU%26usp%3Dchrome_omnibox&usg=AOvVaw3KEdQYFMPa7tgpOn01R5ol) with comments.
>
> It's asking uber login. Can you change share settings?
>
> > remote meeting sync or something else
>
> @kishoreg At least monthly meeting and sync up on project status will be helpful to all...
Yeah our google doc account sharing setting is kind of wanky that it doesn't allow blanket share with everyone outside our organization. I hate it but it is really out of my control. However, just submit a request to view/comment and I can add you as soon as I see it.
We originally plan to have a meeting earlier this month but the pandemic kind of mess up with all of our plans. Maybe we can have a work-group specific remote meeting (eg, upsert meeting with kishore/jackie/chethan etc) in the meantime and do the pinot committee meeting later
@Jackie-Jiang @kishoreg @ChethanUK let me know if you feel meeting is necessary or you prefer me to update the schema first and see if a remote meeting is necessary
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397564813
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -59,13 +65,28 @@
private static final Logger LOGGER = LoggerFactory.getLogger(Schema.class);
private String _schemaName;
+
+ // upsert related config
+ // primary key refers to the column name that is the primary key of this upsert table
+ private String _primaryKey;
Review comment:
right now we don't have support for multiple primary keys. Do you think it will be an important feature to add multiple primary key support right now or you think it is fine to postpone it to later diff?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410382426
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
Review comment:
Can we avoid adding this for now? We can treat the presence of UpsertConfig as enabling upsert for Reatltime-only use cases (that you are handling now). It is therefore implied that the table push style is `APPEND`.
If, in future, we add multiple types of handling upserts, we can always add an enum (
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410382893
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
+ // primary key refers to the column name that is the primary key of this upsert table
+ private List<String> _primaryKeys;
Review comment:
```suggestion
private List<String> _primaryKeyColumns;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r396906006
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerGauge.java
##########
@@ -26,7 +26,8 @@
*
*/
public enum BrokerGauge implements AbstractMetrics.Gauge {
- QUERY_QUOTA_CAPACITY_UTILIZATION_RATE("tables", false), NETTY_CONNECTION_CONNECT_TIME_MS("nettyConnection", true);
+ QUERY_QUOTA_CAPACITY_UTILIZATION_RATE("tables", false), NETTY_CONNECTION_CONNECT_TIME_MS("nettyConnection", true),
+ TABLE_MIN_LOW_WATER_MARK("tables", false);
Review comment:
what does this represent?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410383337
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
+ // primary key refers to the column name that is the primary key of this upsert table
+ private List<String> _primaryKeys;
+ // offset key refers to the column name that we are going to store the offset value to
+ private String _offsetKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validFromKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validUntilKey;
Review comment:
```suggestion
private String _validUntilColumn;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410383235
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
+ // primary key refers to the column name that is the primary key of this upsert table
+ private List<String> _primaryKeys;
+ // offset key refers to the column name that we are going to store the offset value to
+ private String _offsetKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validFromKey;
Review comment:
```suggestion
private String _validFromColumn;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on issue #5175: add
pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-603026218
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=h1) Report
> Merging [#5175](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/56f3e97e71364ec659771dd24f0c8dbad4691934&el=desc) will **decrease** coverage by `0.42%`.
> The diff coverage is `27.77%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #5175 +/- ##
============================================
- Coverage 66.28% 65.86% -0.43%
Complexity 12 12
============================================
Files 1051 1053 +2
Lines 54120 54307 +187
Branches 8064 8103 +39
============================================
- Hits 35874 35767 -107
- Misses 15608 15886 +278
- Partials 2638 2654 +16
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...g/apache/pinot/common/config/TableNameBuilder.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL1RhYmxlTmFtZUJ1aWxkZXIuamF2YQ==) | `86.66% <0.00%> (-13.34%)` | `0.00 <0.00> (ø)` | |
| [...g/apache/pinot/common/metrics/AbstractMetrics.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9BYnN0cmFjdE1ldHJpY3MuamF2YQ==) | `73.33% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...mmon/restlet/resources/TableLowWaterMarksInfo.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzdGxldC9yZXNvdXJjZXMvVGFibGVMb3dXYXRlck1hcmtzSW5mby5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...t/plugin/inputformat/avro/AvroRecordExtractor.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtYXZyby1iYXNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wbHVnaW4vaW5wdXRmb3JtYXQvYXZyby9BdnJvUmVjb3JkRXh0cmFjdG9yLmphdmE=) | `100.00% <ø> (ø)` | `0.00 <0.00> (?)` | |
| [...inot/plugin/inputformat/avro/AvroRecordReader.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtYXZyby1iYXNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wbHVnaW4vaW5wdXRmb3JtYXQvYXZyby9BdnJvUmVjb3JkUmVhZGVyLmphdmE=) | `79.31% <ø> (ø)` | `0.00 <0.00> (?)` | |
| [.../pinot/plugin/inputformat/avro/AvroSchemaUtil.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtYXZyby1iYXNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wbHVnaW4vaW5wdXRmb3JtYXQvYXZyby9BdnJvU2NoZW1hVXRpbC5qYXZh) | `18.51% <ø> (ø)` | `0.00 <0.00> (?)` | |
| [...pache/pinot/plugin/inputformat/avro/AvroUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtYXZyby1iYXNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wbHVnaW4vaW5wdXRmb3JtYXQvYXZyby9BdnJvVXRpbHMuamF2YQ==) | `40.68% <ø> (ø)` | `0.00 <0.00> (?)` | |
| [...rc/main/java/org/apache/pinot/spi/data/Schema.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9TY2hlbWEuamF2YQ==) | `67.00% <14.28%> (-16.05%)` | `0.00 <0.00> (ø)` | |
| [...main/java/org/apache/pinot/spi/data/FieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9GaWVsZFNwZWMuamF2YQ==) | `84.73% <33.33%> (-1.21%)` | `0.00 <0.00> (ø)` | |
| [...org/apache/pinot/common/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `40.98% <38.46%> (+0.16%)` | `0.00 <0.00> (ø)` | |
| ... and [64 more](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=footer). Last update [385c0d3...723f08a](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull
request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397360233
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/TableConfig.java
##########
@@ -75,6 +79,9 @@
private Map<InstancePartitionsType, InstanceAssignmentConfig> _instanceAssignmentConfigMap;
private List<FieldConfig> _fieldConfigList;
+ @JsonPropertyDescription(value = "The update semantic of the table, either append or upsert, default as append")
+ private UpdateSemantic _updateSemantic;
Review comment:
Is this for real-time? We already have push type of either APPEND or REFRESH for offline; aggregateMetrics for real-time. What does UPSERT stand for?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397577584
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/TableConfig.java
##########
@@ -30,17 +30,21 @@
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
+import javax.validation.constraints.Null;
Review comment:
issue with merging, fixed. Thanks for pointing it out
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r402761371
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/TableConfig.java
##########
@@ -470,6 +505,18 @@ public void setInstanceAssignmentConfigMap(
return _fieldConfigList;
}
+ public UpdateSemantic getUpdateSemantic() {
+ return _updateSemantic;
+ }
+
+ public void setUpdateSemantic(UpdateSemantic updateSemantic) {
+ _updateSemantic = updateSemantic;
+ }
+
+ public boolean isTableForUpsert() {
Review comment:
let's avoid adding additional utility methods to config classes if possible
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull
request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397361692
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/TableNameBuilder.java
##########
@@ -132,4 +132,19 @@ public static boolean isOfflineTableResource(String resourceName) {
public static boolean isRealtimeTableResource(String resourceName) {
return REALTIME.tableHasTypeSuffix(resourceName);
}
+
+ /**
+ * ensure that table name ends with type info, if no, create one with the given type
+ * @param tableName the name of the table
+ * @param type the type of the table for it to fill in if the type info is missing
+ * @return the table type name with the type info
+ */
+ public static String ensureTableNameWithType(String tableName, TableType type) {
Review comment:
Please use `TableNameBuilder.forType(type).tableNameWithType(tableName)`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on issue #5175: add pinot
upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-603026218
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=h1) Report
> Merging [#5175](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/56f3e97e71364ec659771dd24f0c8dbad4691934&el=desc) will **decrease** coverage by `0.42%`.
> The diff coverage is `27.77%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #5175 +/- ##
============================================
- Coverage 66.28% 65.86% -0.43%
Complexity 12 12
============================================
Files 1051 1053 +2
Lines 54120 54307 +187
Branches 8064 8103 +39
============================================
- Hits 35874 35767 -107
- Misses 15608 15886 +278
- Partials 2638 2654 +16
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...g/apache/pinot/common/config/TableNameBuilder.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL1RhYmxlTmFtZUJ1aWxkZXIuamF2YQ==) | `86.66% <0.00%> (-13.34%)` | `0.00 <0.00> (ø)` | |
| [...g/apache/pinot/common/metrics/AbstractMetrics.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9BYnN0cmFjdE1ldHJpY3MuamF2YQ==) | `73.33% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...mmon/restlet/resources/TableLowWaterMarksInfo.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzdGxldC9yZXNvdXJjZXMvVGFibGVMb3dXYXRlck1hcmtzSW5mby5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...t/plugin/inputformat/avro/AvroRecordExtractor.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtYXZyby1iYXNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wbHVnaW4vaW5wdXRmb3JtYXQvYXZyby9BdnJvUmVjb3JkRXh0cmFjdG9yLmphdmE=) | `100.00% <ø> (ø)` | `0.00 <0.00> (?)` | |
| [...inot/plugin/inputformat/avro/AvroRecordReader.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtYXZyby1iYXNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wbHVnaW4vaW5wdXRmb3JtYXQvYXZyby9BdnJvUmVjb3JkUmVhZGVyLmphdmE=) | `79.31% <ø> (ø)` | `0.00 <0.00> (?)` | |
| [.../pinot/plugin/inputformat/avro/AvroSchemaUtil.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtYXZyby1iYXNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wbHVnaW4vaW5wdXRmb3JtYXQvYXZyby9BdnJvU2NoZW1hVXRpbC5qYXZh) | `18.51% <ø> (ø)` | `0.00 <0.00> (?)` | |
| [...pache/pinot/plugin/inputformat/avro/AvroUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtYXZyby1iYXNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wbHVnaW4vaW5wdXRmb3JtYXQvYXZyby9BdnJvVXRpbHMuamF2YQ==) | `40.68% <ø> (ø)` | `0.00 <0.00> (?)` | |
| [...rc/main/java/org/apache/pinot/spi/data/Schema.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9TY2hlbWEuamF2YQ==) | `67.00% <14.28%> (-16.05%)` | `0.00 <0.00> (ø)` | |
| [...main/java/org/apache/pinot/spi/data/FieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9GaWVsZFNwZWMuamF2YQ==) | `84.73% <33.33%> (-1.21%)` | `0.00 <0.00> (ø)` | |
| [...org/apache/pinot/common/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `40.98% <38.46%> (+0.16%)` | `0.00 <0.00> (ø)` | |
| ... and [64 more](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=footer). Last update [385c0d3...723f08a](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r396914983
##########
File path: pom.xml
##########
@@ -132,6 +132,9 @@
<log4j.version>2.11.2</log4j.version>
<netty.version>4.1.42.Final</netty.version>
+ <!-- dependency for upsert component -->
Review comment:
Do we need this at compile time?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410380965
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
Review comment:
Can we call this `UpsertConfig`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] ChethanUK commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
ChethanUK commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r398153186
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -59,13 +65,28 @@
private static final Logger LOGGER = LoggerFactory.getLogger(Schema.class);
private String _schemaName;
+
+ // upsert related config
+ // primary key refers to the column name that is the primary key of this upsert table
+ private String _primaryKey;
Review comment:
> don't have support for multiple primary keys
There will be some cases where multiple keys make sense right? Like in clickstream: `userId and timestamp`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397575889
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java
##########
@@ -21,16 +21,17 @@
import com.google.common.annotations.VisibleForTesting;
import com.yammer.metrics.core.MetricName;
import com.yammer.metrics.core.MetricsRegistry;
+import org.apache.pinot.common.Utils;
Review comment:
| Is this for real-time? We already have push type of either APPEND or REFRESH for offline; aggregateMetrics for real-time. What does UPSERT stand for?
replied in earlier comment, basically design for supporting update/insert in pinot realtime ingestion
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull
request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410501155
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
+ // primary key refers to the column name that is the primary key of this upsert table
+ private List<String> _primaryKeys;
+ // offset key refers to the column name that we are going to store the offset value to
+ private String _offsetKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validFromKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validUntilKey;
+
+ public static final String UPSERT_TABLE_CONFIG_VALUE = "upsert";
+ public static final String APPEND_TABLE_CONFIG_VALUE = "append";
+
+ public static final IngestionModeConfig DEFAULT_APPEND_INGESTION_MODE =
+ new IngestionModeConfig(APPEND_TABLE_CONFIG_VALUE, null, null, null,
+ null);
+
+ @JsonCreator
+ public IngestionModeConfig(
+ @JsonProperty(value="ingestionMode") @Nullable String ingestionMode,
+ @JsonProperty(value="primaryKeys") @Nullable List<String> primaryKeys,
Review comment:
If this config is for upsert only, I think all arguments are required
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] ChethanUK commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
ChethanUK commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r398153186
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -59,13 +65,28 @@
private static final Logger LOGGER = LoggerFactory.getLogger(Schema.class);
private String _schemaName;
+
+ // upsert related config
+ // primary key refers to the column name that is the primary key of this upsert table
+ private String _primaryKey;
Review comment:
> don't have support for multiple primary keys
There will be some cases where multiple keys make sense right? Like in clickstream: `userId and timestamp`
Will support for multiple keys be added later on?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397573029
##########
File path: pom.xml
##########
@@ -132,6 +132,9 @@
<log4j.version>2.11.2</log4j.version>
<netty.version>4.1.42.Final</netty.version>
+ <!-- dependency for upsert component -->
Review comment:
moved to submodules
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397577434
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/TableConfig.java
##########
@@ -354,6 +387,7 @@ public ZNRecord toZNRecord()
}
ZNRecord znRecord = new ZNRecord(_tableName);
+ simpleFields.put(UPDATE_SEMANTIC_CONFIG_KEY, _updateSemantic.toString());
Review comment:
updated to follow the convention of rest of the code
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397633097
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -59,13 +65,28 @@
private static final Logger LOGGER = LoggerFactory.getLogger(Schema.class);
private String _schemaName;
+
+ // upsert related config
+ // primary key refers to the column name that is the primary key of this upsert table
+ private String _primaryKey;
Review comment:
Better to design it for multiple, even if the implementation can handle only one primary key
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull
request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410500603
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
Review comment:
Also, please extends BaseJsonConfig, which handles the Json ser-de and common methods like hashCode(), equals() and toString(). You can refer to `RoutingConfig` which is similar to this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397573029
##########
File path: pom.xml
##########
@@ -132,6 +132,9 @@
<log4j.version>2.11.2</log4j.version>
<netty.version>4.1.42.Final</netty.version>
+ <!-- dependency for upsert component -->
Review comment:
moved to submodules, will be reflected in future prs
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull
request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397358004
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/TableConfig.java
##########
@@ -354,6 +387,7 @@ public ZNRecord toZNRecord()
}
ZNRecord znRecord = new ZNRecord(_tableName);
+ simpleFields.put(UPDATE_SEMANTIC_CONFIG_KEY, _updateSemantic.toString());
Review comment:
This can cause NPE
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410384041
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
+ // primary key refers to the column name that is the primary key of this upsert table
+ private List<String> _primaryKeys;
+ // offset key refers to the column name that we are going to store the offset value to
+ private String _offsetKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validFromKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validUntilKey;
+
+ public static final String UPSERT_TABLE_CONFIG_VALUE = "upsert";
+ public static final String APPEND_TABLE_CONFIG_VALUE = "append";
+
+ public static final IngestionModeConfig DEFAULT_APPEND_INGESTION_MODE =
+ new IngestionModeConfig(APPEND_TABLE_CONFIG_VALUE, null, null, null,
+ null);
+
+ @JsonCreator
+ public IngestionModeConfig(
+ @JsonProperty(value="ingestionMode") @Nullable String ingestionMode,
+ @JsonProperty(value="primaryKeys") @Nullable List<String> primaryKeys,
+ @JsonProperty(value="offsetKey") @Nullable String offsetKey,
+ @JsonProperty(value="validFromKey") @Nullable String validFromKey,
+ @JsonProperty(value="validUntilKey") @Nullable String validUntilKey) {
+ if (StringUtils.isEmpty(ingestionMode)) {
+ _ingestionMode = APPEND_TABLE_CONFIG_VALUE;
+ } else {
+ _ingestionMode = ingestionMode.toLowerCase();
+ }
+ if (primaryKeys == null) {
+ _primaryKeys = ImmutableList.of();
+ } else {
+ _primaryKeys = primaryKeys;
+ }
+ _offsetKey = offsetKey;
+ _validFromKey = validFromKey;
+ _validUntilKey = validUntilKey;
+
+ if (UPSERT_TABLE_CONFIG_VALUE.equals(_ingestionMode)) {
+ Preconditions.checkState(_primaryKeys.size() == 1,
+ "pinot upsert require one and only one primary key");
Review comment:
```suggestion
"Upsert feature supports only one primary key");
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r396905801
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/TableNameBuilder.java
##########
@@ -132,4 +132,19 @@ public static boolean isOfflineTableResource(String resourceName) {
public static boolean isRealtimeTableResource(String resourceName) {
return REALTIME.tableHasTypeSuffix(resourceName);
}
+
+ /**
+ * ensure that table name ends with type info, if no, create one with the given type
+ * @param tableName the name of the table
+ * @param type the type of the table for it to fill in if the type info is missing
+ * @return the table type name with the type info
+ */
+ public static String ensureTableNameWithType(String tableName, TableType type) {
Review comment:
where is this used?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r396907773
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -59,13 +65,28 @@
private static final Logger LOGGER = LoggerFactory.getLogger(Schema.class);
private String _schemaName;
+
+ // upsert related config
+ // primary key refers to the column name that is the primary key of this upsert table
+ private String _primaryKey;
Review comment:
what if the primary key is made up of two columns?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r401996265
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -378,4 +407,23 @@ public ServerType getServerType() {
@Deprecated
public static final String TABLE_NAME = "segment.table.name";
}
+
+ public enum UpdateSemantic {
+ APPEND,
Review comment:
Please add comments on each of these. What does APPEND mean, vs what UPSERT means. Feel free to point to the design doc
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r402613815
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -59,13 +65,28 @@
private static final Logger LOGGER = LoggerFactory.getLogger(Schema.class);
private String _schemaName;
+
+ // upsert related config
+ // primary key refers to the column name that is the primary key of this upsert table
+ private String _primaryKey;
Review comment:
@kishoreg @ChethanUK I have changed the primary key in schema config to be a list instead of a single string. For now we are still performing check to ensure that primary key will be one and only one. However we should be able to change that once we have mulitple-primary-key support in place
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410383571
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
+ // primary key refers to the column name that is the primary key of this upsert table
+ private List<String> _primaryKeys;
+ // offset key refers to the column name that we are going to store the offset value to
+ private String _offsetKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validFromKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validUntilKey;
+
+ public static final String UPSERT_TABLE_CONFIG_VALUE = "upsert";
Review comment:
we should not need these two for now
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397563786
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/TableConfig.java
##########
@@ -75,6 +79,9 @@
private Map<InstancePartitionsType, InstanceAssignmentConfig> _instanceAssignmentConfigMap;
private List<FieldConfig> _fieldConfigList;
+ @JsonPropertyDescription(value = "The update semantic of the table, either append or upsert, default as append")
+ private UpdateSemantic _updateSemantic;
Review comment:
@kishoreg I am thinking if using enum here will be more flexible in the case of future features changes or whatnot. Boolean seems to be only limiting two options and it might limit what other changes we might want to do with pinot. Please let me what you think. ingestionMode definitely sounds more explicit than what I am using here and I will definite consider about this.
@Jackie-Jiang yes. So basically it is previous old design about we want to support update in pinot realtime ingestion by adding override by primary key (eg, a second message in Kafka with the same primary key represents the update of existing key instead of two records with the same primary key). Upsert stands for this new ingestion semantic (insert if no duplicated primary key, update if duplicate primary key). This feature will be applying to realtime table only
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on issue #5175: add pinot
upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on issue #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-603604130
> Can you please link the design doc in the pr description so that it is easier to understand the concept of upsert?
> Since this pr is touching the core configs, we need to spend some time discussing how to design the config keys because they are very hard to change in the future
Linked the design doc in the description. also [google doc design doc](https://www.google.com/url?sa=t&rct=j&esrc=s&source=appssearch&uact=8&cd=0&cad=rja&q&sig2=4ypfJoFtqjSdua_7dziWCQ&ved=0ahUKEwjVhpqIybToAhXDHWIKHWFcBVs4ABABKAAwAA&url=https://drive.google.com/a/uber.com/open?id%3D1SFFir7ByxCff-aVYxQeTHpNhPXeP5q7P4g_6O2iNGgU%26usp%3Dchrome_omnibox&usg=AOvVaw3KEdQYFMPa7tgpOn01R5ol) with comments. We have discussed this a long time back but I definitely open to go through it again as things have already changed. Do you think we should do some remote meeting sync or something else?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410383081
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
+ // primary key refers to the column name that is the primary key of this upsert table
+ private List<String> _primaryKeys;
+ // offset key refers to the column name that we are going to store the offset value to
+ private String _offsetKey;
Review comment:
```suggestion
private String _offsetColumn;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410385779
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
+ // primary key refers to the column name that is the primary key of this upsert table
+ private List<String> _primaryKeys;
+ // offset key refers to the column name that we are going to store the offset value to
+ private String _offsetKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validFromKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validUntilKey;
+
+ public static final String UPSERT_TABLE_CONFIG_VALUE = "upsert";
+ public static final String APPEND_TABLE_CONFIG_VALUE = "append";
+
+ public static final IngestionModeConfig DEFAULT_APPEND_INGESTION_MODE =
+ new IngestionModeConfig(APPEND_TABLE_CONFIG_VALUE, null, null, null,
+ null);
+
+ @JsonCreator
+ public IngestionModeConfig(
+ @JsonProperty(value="ingestionMode") @Nullable String ingestionMode,
+ @JsonProperty(value="primaryKeys") @Nullable List<String> primaryKeys,
+ @JsonProperty(value="offsetKey") @Nullable String offsetKey,
+ @JsonProperty(value="validFromKey") @Nullable String validFromKey,
+ @JsonProperty(value="validUntilKey") @Nullable String validUntilKey) {
+ if (StringUtils.isEmpty(ingestionMode)) {
+ _ingestionMode = APPEND_TABLE_CONFIG_VALUE;
+ } else {
+ _ingestionMode = ingestionMode.toLowerCase();
+ }
+ if (primaryKeys == null) {
+ _primaryKeys = ImmutableList.of();
+ } else {
+ _primaryKeys = primaryKeys;
+ }
+ _offsetKey = offsetKey;
+ _validFromKey = validFromKey;
+ _validUntilKey = validUntilKey;
+
+ if (UPSERT_TABLE_CONFIG_VALUE.equals(_ingestionMode)) {
+ Preconditions.checkState(_primaryKeys.size() == 1,
+ "pinot upsert require one and only one primary key");
+ Preconditions.checkState(StringUtils.isNotEmpty(_offsetKey),
+ "pinot upsert require one offset key");
+ Preconditions.checkState(StringUtils.isNotEmpty(_validFromKey),
+ "pinot upsert require one validFrom key");
+ Preconditions.checkState(StringUtils.isNotEmpty(_validUntilKey),
+ "pinot upsert require one validUntil key");
Review comment:
```suggestion
"Upsert feature requires \"validUntilColumn\" to be set");
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull
request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397355590
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/TableConfig.java
##########
@@ -30,17 +30,21 @@
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
+import javax.validation.constraints.Null;
Review comment:
Wrong import
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r401996027
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -308,6 +320,23 @@ public ServerType getServerType() {
public static final String PREFIX_OF_CONFIG_OF_PINOT_CRYPTER = "crypter";
}
+ public static class Grigio {
Review comment:
Can we change the name of this class like I had suggested before?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397571798
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -631,4 +751,50 @@ public int hashCode() {
result = EqualityUtils.hashCodeOf(result, _dateTimeFieldSpecs);
return result;
}
+
+ public boolean isVirtualColumn(String columnName) {
+ return columnName.startsWith("$") || (getFieldSpecFor(columnName).getVirtualColumnProvider() != null
+ && !getFieldSpecFor(columnName).getVirtualColumnProvider().isEmpty());
+ }
+
+
+ @JsonIgnore
+ public static byte[] getByteArrayFromField(Object value, DimensionFieldSpec fieldSpec) {
Review comment:
done.
It is used for converting a pinot column value into the appropriate binary representation for upsert primary key field. (make sure all primary key will become a byte array)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on issue #5175: add pinot
upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
kishoreg commented on issue #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-603667466
One concern here would be that the update related configuration appears in multiple places. One thing that can simplify this PR is to add a UpdateConfig under TableConfig and move everything under that. That allows you to mark that config as beta/unstable and iterate quickly
- updatesemantic (or the new name)
- primarykey
- offsetkey
-
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410386203
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/TableConfig.java
##########
@@ -44,6 +45,7 @@
public static final String QUERY_CONFIG_KEY = "query";
public static final String INSTANCE_ASSIGNMENT_CONFIG_MAP_KEY = "instanceAssignmentConfigMap";
public static final String FIELD_CONFIG_LIST_KEY = "fieldConfigList";
+ public static final String INGESTION_MODE_CONFIG_KEY = "ingestionModeConfig";
Review comment:
```suggestion
public static final String UPSERT_CONFIG_KEY = "upsertConfig";
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r396905344
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/TableConfig.java
##########
@@ -75,6 +79,9 @@
private Map<InstancePartitionsType, InstanceAssignmentConfig> _instanceAssignmentConfigMap;
private List<FieldConfig> _fieldConfigList;
+ @JsonPropertyDescription(value = "The update semantic of the table, either append or upsert, default as append")
+ private UpdateSemantic _updateSemantic;
Review comment:
rename to IngestMode or just have a boolean enableUpsert?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on issue #5175: add pinot
upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on issue #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-615037586
@kishoreg @mcvsubbu I have updated the diff per suggestion you guys gave in the last meeting, please feel free to take a look
@Jackie-Jiang I have updated the diff to make sure #1: it is only related table config changes required by pinot upsert feature (see [doc](https://docs.google.com/document/d/1SFFir7ByxCff-aVYxQeTHpNhPXeP5q7P4g_6O2iNGgU/edit?usp=sharing) for details) and #2 push other pinot-common related changes for pinot upsert features to later PRs
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410385665
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
+ // primary key refers to the column name that is the primary key of this upsert table
+ private List<String> _primaryKeys;
+ // offset key refers to the column name that we are going to store the offset value to
+ private String _offsetKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validFromKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validUntilKey;
+
+ public static final String UPSERT_TABLE_CONFIG_VALUE = "upsert";
+ public static final String APPEND_TABLE_CONFIG_VALUE = "append";
+
+ public static final IngestionModeConfig DEFAULT_APPEND_INGESTION_MODE =
+ new IngestionModeConfig(APPEND_TABLE_CONFIG_VALUE, null, null, null,
+ null);
+
+ @JsonCreator
+ public IngestionModeConfig(
+ @JsonProperty(value="ingestionMode") @Nullable String ingestionMode,
+ @JsonProperty(value="primaryKeys") @Nullable List<String> primaryKeys,
+ @JsonProperty(value="offsetKey") @Nullable String offsetKey,
+ @JsonProperty(value="validFromKey") @Nullable String validFromKey,
+ @JsonProperty(value="validUntilKey") @Nullable String validUntilKey) {
+ if (StringUtils.isEmpty(ingestionMode)) {
+ _ingestionMode = APPEND_TABLE_CONFIG_VALUE;
+ } else {
+ _ingestionMode = ingestionMode.toLowerCase();
+ }
+ if (primaryKeys == null) {
+ _primaryKeys = ImmutableList.of();
+ } else {
+ _primaryKeys = primaryKeys;
+ }
+ _offsetKey = offsetKey;
+ _validFromKey = validFromKey;
+ _validUntilKey = validUntilKey;
+
+ if (UPSERT_TABLE_CONFIG_VALUE.equals(_ingestionMode)) {
+ Preconditions.checkState(_primaryKeys.size() == 1,
+ "pinot upsert require one and only one primary key");
+ Preconditions.checkState(StringUtils.isNotEmpty(_offsetKey),
+ "pinot upsert require one offset key");
+ Preconditions.checkState(StringUtils.isNotEmpty(_validFromKey),
+ "pinot upsert require one validFrom key");
Review comment:
```suggestion
"Upsert feature requires \"validFromColumn\" to be set");
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull
request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410500970
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
+ // primary key refers to the column name that is the primary key of this upsert table
+ private List<String> _primaryKeys;
Review comment:
+1. You can also make it final. Same for other places
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410384495
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
+ // new config key to indicate if a table is for upsert. If this is define as upsert, it would be an upsert table
+ // for other value it would be append value for now
+ private String _ingestionMode;
+ // primary key refers to the column name that is the primary key of this upsert table
+ private List<String> _primaryKeys;
+ // offset key refers to the column name that we are going to store the offset value to
+ private String _offsetKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validFromKey;
+ // validFrom key refers to the column name that we are going to store the validFrom value to
+ private String _validUntilKey;
+
+ public static final String UPSERT_TABLE_CONFIG_VALUE = "upsert";
+ public static final String APPEND_TABLE_CONFIG_VALUE = "append";
+
+ public static final IngestionModeConfig DEFAULT_APPEND_INGESTION_MODE =
+ new IngestionModeConfig(APPEND_TABLE_CONFIG_VALUE, null, null, null,
+ null);
+
+ @JsonCreator
+ public IngestionModeConfig(
+ @JsonProperty(value="ingestionMode") @Nullable String ingestionMode,
+ @JsonProperty(value="primaryKeys") @Nullable List<String> primaryKeys,
+ @JsonProperty(value="offsetKey") @Nullable String offsetKey,
+ @JsonProperty(value="validFromKey") @Nullable String validFromKey,
+ @JsonProperty(value="validUntilKey") @Nullable String validUntilKey) {
+ if (StringUtils.isEmpty(ingestionMode)) {
+ _ingestionMode = APPEND_TABLE_CONFIG_VALUE;
+ } else {
+ _ingestionMode = ingestionMode.toLowerCase();
+ }
+ if (primaryKeys == null) {
+ _primaryKeys = ImmutableList.of();
+ } else {
+ _primaryKeys = primaryKeys;
+ }
+ _offsetKey = offsetKey;
+ _validFromKey = validFromKey;
+ _validUntilKey = validUntilKey;
+
+ if (UPSERT_TABLE_CONFIG_VALUE.equals(_ingestionMode)) {
+ Preconditions.checkState(_primaryKeys.size() == 1,
+ "pinot upsert require one and only one primary key");
+ Preconditions.checkState(StringUtils.isNotEmpty(_offsetKey),
+ "pinot upsert require one offset key");
Review comment:
```suggestion
"Upsert feature requires \"offsetColumn\" to be set");
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r396914359
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -59,13 +65,28 @@
private static final Logger LOGGER = LoggerFactory.getLogger(Schema.class);
private String _schemaName;
+
+ // upsert related config
+ // primary key refers to the column name that is the primary key of this upsert table
+ private String _primaryKey;
+ // offset key refers to the column name that we are going to store the offset value to
+ private String _offsetKey;
Review comment:
do we need all these things in Schema? What if we create a UpsertConfig in TableConfig to provide all the necessary information.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull
request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r410500071
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/IngestionModeConfig.java
##########
@@ -0,0 +1,152 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class IngestionModeConfig {
Review comment:
+1. I think this config is only for upsert feature, and not applicable for general ingestion. When this config exist, we can assume the upsert feature is on.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] ChethanUK commented on issue #5175: add pinot
upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
ChethanUK commented on issue #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-604074970
> also [google doc design doc](https://www.google.com/url?sa=t&rct=j&esrc=s&source=appssearch&uact=8&cd=0&cad=rja&q&sig2=4ypfJoFtqjSdua_7dziWCQ&ved=0ahUKEwjVhpqIybToAhXDHWIKHWFcBVs4ABABKAAwAA&url=https://drive.google.com/a/uber.com/open?id%3D1SFFir7ByxCff-aVYxQeTHpNhPXeP5q7P4g_6O2iNGgU%26usp%3Dchrome_omnibox&usg=AOvVaw3KEdQYFMPa7tgpOn01R5ol) with comments.
It's asking uber login. Can you change share settings?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r401940844
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/TableNameBuilder.java
##########
@@ -132,4 +132,19 @@ public static boolean isOfflineTableResource(String resourceName) {
public static boolean isRealtimeTableResource(String resourceName) {
return REALTIME.tableHasTypeSuffix(resourceName);
}
+
+ /**
+ * ensure that table name ends with type info, if no, create one with the given type
+ * @param tableName the name of the table
+ * @param type the type of the table for it to fill in if the type info is missing
+ * @return the table type name with the type info
+ */
+ public static String ensureTableNameWithType(String tableName, TableType type) {
Review comment:
removed this method, @kishoreg this method is used in other components that I will submit PR later
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r396914763
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -631,4 +751,50 @@ public int hashCode() {
result = EqualityUtils.hashCodeOf(result, _dateTimeFieldSpecs);
return result;
}
+
+ public boolean isVirtualColumn(String columnName) {
+ return columnName.startsWith("$") || (getFieldSpecFor(columnName).getVirtualColumnProvider() != null
+ && !getFieldSpecFor(columnName).getVirtualColumnProvider().isEmpty());
+ }
+
+
+ @JsonIgnore
+ public static byte[] getByteArrayFromField(Object value, DimensionFieldSpec fieldSpec) {
Review comment:
why is this needed? it will be great if we can avoid static methods in config objects. We can probably move this SchemaUtil
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] ChethanUK edited a comment on issue #5175: add
pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
ChethanUK edited a comment on issue #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-604074970
> also [google doc design doc](https://www.google.com/url?sa=t&rct=j&esrc=s&source=appssearch&uact=8&cd=0&cad=rja&q&sig2=4ypfJoFtqjSdua_7dziWCQ&ved=0ahUKEwjVhpqIybToAhXDHWIKHWFcBVs4ABABKAAwAA&url=https://drive.google.com/a/uber.com/open?id%3D1SFFir7ByxCff-aVYxQeTHpNhPXeP5q7P4g_6O2iNGgU%26usp%3Dchrome_omnibox&usg=AOvVaw3KEdQYFMPa7tgpOn01R5ol) with comments.
It's asking uber login. Can you change share settings?
> remote meeting sync or something else
@kishoreg At least monthly meeting and sync up on project status will be helpful to all...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull
request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397361994
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java
##########
@@ -21,16 +21,17 @@
import com.google.common.annotations.VisibleForTesting;
import com.yammer.metrics.core.MetricName;
import com.yammer.metrics.core.MetricsRegistry;
+import org.apache.pinot.common.Utils;
Review comment:
Wrong order, please revert
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397567501
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -59,13 +65,28 @@
private static final Logger LOGGER = LoggerFactory.getLogger(Schema.class);
private String _schemaName;
+
+ // upsert related config
+ // primary key refers to the column name that is the primary key of this upsert table
+ private String _primaryKey;
+ // offset key refers to the column name that we are going to store the offset value to
+ private String _offsetKey;
Review comment:
definitely would be better this way, will submit a change later to update this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r397564517
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerGauge.java
##########
@@ -26,7 +26,8 @@
*
*/
public enum BrokerGauge implements AbstractMetrics.Gauge {
- QUERY_QUOTA_CAPACITY_UTILIZATION_RATE("tables", false), NETTY_CONNECTION_CONNECT_TIME_MS("nettyConnection", true);
+ QUERY_QUOTA_CAPACITY_UTILIZATION_RATE("tables", false), NETTY_CONNECTION_CONNECT_TIME_MS("nettyConnection", true),
+ TABLE_MIN_LOW_WATER_MARK("tables", false);
Review comment:
It represents how many table pinot broker is collecting the metrics for. The exact usage of the metrics will show up in later diffs
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5175: add pinot upsert features to pinot common
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r396913772
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -59,13 +65,28 @@
private static final Logger LOGGER = LoggerFactory.getLogger(Schema.class);
private String _schemaName;
+
+ // upsert related config
+ // primary key refers to the column name that is the primary key of this upsert table
+ private String _primaryKey;
Review comment:
or multiple
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5175:
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r414794546
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/UpsertConfig.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class UpsertConfig extends BaseJsonConfig {
+ // names of the columns that used as primary keys of an upsert table
+ private final List<String> _primaryKeyColumns;
+ // name of the column that we are going to store the offset value to
+ private final String _offsetColumn;
+ // name of the virtual column that we are going to store the validFrom value to
+ private final String _validFromColumn;
+ // name of the virtual column that we are going to store the validUntil value to
+ private final String _validUntilColumn;
+
+ @JsonCreator
+ public UpsertConfig(
+ @JsonProperty(value="primaryKeyColumns") List<String> primaryKeyColumns,
+ @JsonProperty(value="offsetColumn") String offsetColumn,
+ @JsonProperty(value="validFromColumn") String validFromColumn,
+ @JsonProperty(value="validUntilColumn") String validUntilColumn) {
+
+ _primaryKeyColumns = primaryKeyColumns;
+ _offsetColumn = offsetColumn;
+ _validFromColumn = validFromColumn;
+ _validUntilColumn = validUntilColumn;
+
+ Preconditions.checkState(_primaryKeyColumns.size() == 1,
+ "upsert feature support only one primary key column");
Review comment:
```suggestion
"Upsert feature supports only one primary key column");
```
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/UpsertConfig.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class UpsertConfig extends BaseJsonConfig {
+ // names of the columns that used as primary keys of an upsert table
+ private final List<String> _primaryKeyColumns;
+ // name of the column that we are going to store the offset value to
+ private final String _offsetColumn;
+ // name of the virtual column that we are going to store the validFrom value to
+ private final String _validFromColumn;
+ // name of the virtual column that we are going to store the validUntil value to
+ private final String _validUntilColumn;
+
+ @JsonCreator
+ public UpsertConfig(
+ @JsonProperty(value="primaryKeyColumns") List<String> primaryKeyColumns,
+ @JsonProperty(value="offsetColumn") String offsetColumn,
+ @JsonProperty(value="validFromColumn") String validFromColumn,
+ @JsonProperty(value="validUntilColumn") String validUntilColumn) {
+
+ _primaryKeyColumns = primaryKeyColumns;
+ _offsetColumn = offsetColumn;
+ _validFromColumn = validFromColumn;
+ _validUntilColumn = validUntilColumn;
+
+ Preconditions.checkState(_primaryKeyColumns.size() == 1,
+ "upsert feature support only one primary key column");
+ Preconditions.checkState(StringUtils.isNotEmpty(_offsetColumn),
+ "upsert feature requires \"offsetColumn\" to be set ");
Review comment:
```suggestion
"Upsert feature requires \"offsetColumn\" to be set ");
```
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/UpsertConfig.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class UpsertConfig extends BaseJsonConfig {
+ // names of the columns that used as primary keys of an upsert table
+ private final List<String> _primaryKeyColumns;
+ // name of the column that we are going to store the offset value to
+ private final String _offsetColumn;
+ // name of the virtual column that we are going to store the validFrom value to
+ private final String _validFromColumn;
+ // name of the virtual column that we are going to store the validUntil value to
+ private final String _validUntilColumn;
+
+ @JsonCreator
+ public UpsertConfig(
+ @JsonProperty(value="primaryKeyColumns") List<String> primaryKeyColumns,
+ @JsonProperty(value="offsetColumn") String offsetColumn,
+ @JsonProperty(value="validFromColumn") String validFromColumn,
+ @JsonProperty(value="validUntilColumn") String validUntilColumn) {
+
+ _primaryKeyColumns = primaryKeyColumns;
+ _offsetColumn = offsetColumn;
+ _validFromColumn = validFromColumn;
+ _validUntilColumn = validUntilColumn;
+
+ Preconditions.checkState(_primaryKeyColumns.size() == 1,
+ "upsert feature support only one primary key column");
+ Preconditions.checkState(StringUtils.isNotEmpty(_offsetColumn),
+ "upsert feature requires \"offsetColumn\" to be set ");
+ Preconditions.checkState(StringUtils.isNotEmpty(_validFromColumn),
+ "upsert feature requires \"validFromColumn\" to be set");
+ Preconditions.checkState(StringUtils.isNotEmpty(_validUntilColumn),
+ "upsert feature requires \"validUntilColumn\" to be set");
Review comment:
```suggestion
"Upsert feature requires \"validUntilColumn\" to be set");
```
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/UpsertConfig.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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.pinot.spi.config;
+
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang.StringUtils;
+
+import javax.annotation.Nullable;
+import java.util.List;
+
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class UpsertConfig extends BaseJsonConfig {
+ // names of the columns that used as primary keys of an upsert table
+ private final List<String> _primaryKeyColumns;
+ // name of the column that we are going to store the offset value to
+ private final String _offsetColumn;
+ // name of the virtual column that we are going to store the validFrom value to
+ private final String _validFromColumn;
+ // name of the virtual column that we are going to store the validUntil value to
+ private final String _validUntilColumn;
+
+ @JsonCreator
+ public UpsertConfig(
+ @JsonProperty(value="primaryKeyColumns") List<String> primaryKeyColumns,
+ @JsonProperty(value="offsetColumn") String offsetColumn,
+ @JsonProperty(value="validFromColumn") String validFromColumn,
+ @JsonProperty(value="validUntilColumn") String validUntilColumn) {
+
+ _primaryKeyColumns = primaryKeyColumns;
+ _offsetColumn = offsetColumn;
+ _validFromColumn = validFromColumn;
+ _validUntilColumn = validUntilColumn;
+
+ Preconditions.checkState(_primaryKeyColumns.size() == 1,
+ "upsert feature support only one primary key column");
+ Preconditions.checkState(StringUtils.isNotEmpty(_offsetColumn),
+ "upsert feature requires \"offsetColumn\" to be set ");
+ Preconditions.checkState(StringUtils.isNotEmpty(_validFromColumn),
+ "upsert feature requires \"validFromColumn\" to be set");
Review comment:
```suggestion
"Upsert feature requires \"validFromColumn\" to be set");
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #5175:
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-603026218
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=h1) Report
> Merging [#5175](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/d8a2705a69e3d785438126f74f2dc5379c89d4c9&el=desc) will **decrease** coverage by `9.25%`.
> The diff coverage is `92.85%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #5175 +/- ##
==========================================
- Coverage 66.20% 56.95% -9.26%
==========================================
Files 1067 1068 +1
Lines 54532 54570 +38
Branches 8128 8133 +5
==========================================
- Hits 36104 31080 -5024
- Misses 15786 21070 +5284
+ Partials 2642 2420 -222
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ain/java/org/apache/pinot/spi/utils/JsonUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvSnNvblV0aWxzLmphdmE=) | `30.00% <ø> (ø)` | |
| [...org/apache/pinot/spi/config/table/TableConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlQ29uZmlnLmphdmE=) | `64.91% <60.00%> (-1.13%)` | :arrow_down: |
| [...he/pinot/common/utils/config/TableConfigUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvY29uZmlnL1RhYmxlQ29uZmlnVXRpbHMuamF2YQ==) | `86.31% <100.00%> (-3.46%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/config/UpsertConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL1Vwc2VydENvbmZpZy5qYXZh) | `100.00% <100.00%> (ø)` | |
| [...he/pinot/spi/utils/builder/TableConfigBuilder.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvYnVpbGRlci9UYWJsZUNvbmZpZ0J1aWxkZXIuamF2YQ==) | `73.39% <100.00%> (+0.49%)` | :arrow_up: |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...he/pinot/core/query/reduce/ComparisonFunction.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvQ29tcGFyaXNvbkZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [316 more](https://codecov.io/gh/apache/incubator-pinot/pull/5175/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=footer). Last update [d8a2705...a5fd4b9](https://codecov.io/gh/apache/incubator-pinot/pull/5175?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5175:
URL: https://github.com/apache/incubator-pinot/pull/5175#discussion_r413222162
##########
File path: pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java
##########
@@ -367,4 +381,12 @@ private void checkFieldConfig(TableConfig tableConfig) {
assertNull(secondFieldConfig.getIndexType());
assertNull(secondFieldConfig.getProperties());
}
+
+ private void checkTableConfigWithUpsertConfig(TableConfig tableConfig, TableConfig tableConfigToCompare) {
Review comment:
Can you make this similar to other config check? Pass in a table config and check the actual value inside the config. Then serialize and deserialize (both string and ZNRecord) and check again
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
##########
@@ -116,8 +117,16 @@ public static TableConfig fromZNRecord(ZNRecord znRecord)
});
}
+ UpsertConfig upsertConfig = null;
+ String upsertConfigString = simpleFields.get(TableConfig.UPSERT_CONFIG_KEY);
+ if (upsertConfigString != null) {
+ upsertConfig = JsonUtils.stringToObject(upsertConfigString, UpsertConfig.class);
+ }
+
Review comment:
(nit) extra empty line
##########
File path: pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java
##########
@@ -231,6 +234,17 @@ public void testSerDe()
assertEquals(tableConfigToCompare, tableConfig);
checkFieldConfig(tableConfigToCompare);
}
+ {
+ // with upsert config
+ UpsertConfig upsertConfig = new UpsertConfig(ImmutableList.of("pk"), "offset",
Review comment:
Prefer using `Collections.singletonList()`
##########
File path: pinot-spi/src/test/java/org/apache/pinot/spi/config/UpsertConfigTest.java
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.pinot.spi.config;
+
+import com.google.common.collect.ImmutableList;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.*;
+
+public class UpsertConfigTest {
+
+ @Test
+ public void testUpsertConfig() {
+ UpsertConfig upsertConfig;
+
+ // test regular upsert table
+ upsertConfig = new UpsertConfig(ImmutableList.of("primaryKey"), "offset", "validFrom",
+ "validUntil");
+ assertEquals(1, upsertConfig.getPrimaryKeyColumns().size());
+ assertEquals("primaryKey", upsertConfig.getPrimaryKeyColumns().get(0));
+ assertEquals("offset", upsertConfig.getOffsetColumn());
+ assertEquals("validFrom", upsertConfig.getValidFromColumn());
+ assertEquals("validUntil", upsertConfig.getValidUntilColumn());
+
+ // test sanity check
+ try {
+ upsertConfig = new UpsertConfig(null,
+ "offset", "validFrom", "validUntil");
+ fail();
+ } catch (RuntimeException ex) {}
+
+ try {
+ upsertConfig = new UpsertConfig(ImmutableList.of("pk1", "pk2"),
+ "offset", "validFrom", "validUntil");
+ fail();
+ } catch (RuntimeException ex) {}
+
+ try {
+ upsertConfig = new UpsertConfig(ImmutableList.of("pk1"),
+ null, "validFrom", "validUntil");
+ fail();
+ } catch (RuntimeException ex) {}
+
+ try {
+ upsertConfig = new UpsertConfig(ImmutableList.of("pk1"),
+ "offset", null, "validUntil");
+ fail();
+ } catch (RuntimeException ex) {}
+
+ try {
+ upsertConfig = new UpsertConfig(ImmutableList.of("pk1"),
+ "offset", "validFrom", null);
+ fail();
+ } catch (RuntimeException ex) {}
+ }
+}
Review comment:
(nit) empty line
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5175:
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-619233850
@jamesyfshao I am good with this PR, thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on issue #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on issue #5175:
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-617566160
@mcvsubbu @Jackie-Jiang updated per your feedback and make the config to be used only if the table is for upsert and clean up on naming, please take a look when you have time
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on pull request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on pull request #5175:
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-619229065
> minor comments, other than that, lgtm thanks
merged the comments diff, thanks for checking on those details. Do you think any further changes you want? It would be great if you can change your review status from requesting-changes so I can start merging it in
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jamesyfshao commented on pull request #5175: add pinot upsert features to pinot common
Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on pull request #5175:
URL: https://github.com/apache/incubator-pinot/pull/5175#issuecomment-619169444
hi @mcvsubbu we have updated the pr following the comments. Appreciate if you can take a look to see if anything else you feel we need to update further
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org