[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #4622: Standardize the Dictionary interface, ensure the BYTES support
Jackie-Jiang opened a new pull request #4622: Standardize the Dictionary interface, ensure the BYTES support URL: https://github.com/apache/incubator-pinot/pull/4622 - Type conversion between INT/LONG/FLOAT/DOUBLE/STRING are allowed - Type conversion between STRING/BYTES are allowed - Push down and enhance the MutableDictionary range predicate evaluation - Add documentation for the contract of Dictionary interface 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[incubator-pinot] branch enhance_dictionary created (now 404c79f)
This is an automated email from the ASF dual-hosted git repository. jackie pushed a change to branch enhance_dictionary in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git. at 404c79f Standardize the Dictionary interface, ensure the BYTES support No new revisions were added by this update. - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on issue #4585: Presence vector
icefury71 commented on issue #4585: Presence vector URL: https://github.com/apache/incubator-pinot/pull/4585#issuecomment-532923579 > Sorry for the delay. I was out for a while for the conference. > > Would you add the following to the commit message? > > 1. Link #4230 for the reference. After I read the issue, this pr made much more sense. > 2. Add the description on how presence vector is populated. By reading the code, it seems that we generate presence vector by default. > 3. Can you also add a little bit more explanation to the commit message on when you filter out NULL values? (e.g. when predicate is added by user -- column != NULL...) > > I think that most comments are minor & style issues. I will do one more final review after this. Thanks for pointing that out. Updated description. Please take a look 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #4585: Presence vector
icefury71 commented on a change in pull request #4585: Presence vector
URL: https://github.com/apache/incubator-pinot/pull/4585#discussion_r325952566
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/docidsets/BitmapDocIdSet.java
##
@@ -33,7 +36,8 @@
public BitmapDocIdSet(ImmutableRoaringBitmap[] bitmaps, int startDocId, int
endDocId, boolean exclusive) {
int numBitmaps = bitmaps.length;
if (numBitmaps > 1) {
- MutableRoaringBitmap orBitmap = MutableRoaringBitmap.or(bitmaps);
+ Iterator iterator = Arrays.asList(bitmaps).iterator();
+ MutableRoaringBitmap orBitmap = MutableRoaringBitmap.or(iterator,
startDocId, endDocId + 1);
Review comment:
@kishoreg : can you please clarify ? I'm not sure we need this change for
presence vector.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jihaozh commented on issue #4619: [TE] Add data set display name and show it on UI
jihaozh commented on issue #4619: [TE] Add data set display name and show it on UI URL: https://github.com/apache/incubator-pinot/pull/4619#issuecomment-532909223 > It looks good, but it's failing CI tests Thanks. Just pushed the fix for the front end tests. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jihaozh commented on a change in pull request #4621: [TE] fix the last time stamp filter out the generated anomalies
jihaozh commented on a change in pull request #4621: [TE] fix the last time
stamp filter out the generated anomalies
URL: https://github.com/apache/incubator-pinot/pull/4621#discussion_r325931369
##
File path:
thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/DetectionResource.java
##
@@ -476,7 +477,11 @@ public Response detectionReplay(
}
for (MergedAnomalyResultDTO anomaly : result.getAnomalies()) {
-anomaly.setAnomalyResultSource(AnomalyResultSource.ANOMALY_REPLAY);
+// if would like to skip sending the alert emails, the anomalies
should be marked with REPLAY
+// otherwise, should be left as the default source
+if (!sendEmail) {
+ anomaly.setAnomalyResultSource(AnomalyResultSource.ANOMALY_REPLAY);
Review comment:
If the source set to ANOMALY_REPLAY, the alerter will skip sending it.
Actually, that's the only place this flag is used in the system. If that's not
set, the alerter can pick up the anomaly and sent it out.
I think we should have an option to send the anomalies generated by replay.
This is useful when the system missed an anomaly but the email is still
important for the customer.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jihaozh commented on a change in pull request #4620: [TE] [Notification] Refactor email filters to a generalized alert filter
jihaozh commented on a change in pull request #4620: [TE] [Notification]
Refactor email filters to a generalized alert filter
URL: https://github.com/apache/incubator-pinot/pull/4620#discussion_r325930055
##
File path:
thirdeye/thirdeye-pinot/src/test/java/org/apache/pinot/thirdeye/detection/alert/filter/AlertFilterUtils.java
##
@@ -0,0 +1,55 @@
+/**
+ * Copyright (C) 2014-2018 LinkedIn Corp. (pinot-c...@linkedin.com)
+ *
+ * Licensed 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.thirdeye.detection.alert.filter;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import
org.apache.pinot.thirdeye.detection.alert.DetectionAlertFilterNotification;
+
+
+public class AlertFilterUtils {
+
+ private static final String PROP_RECIPIENTS = "recipients";
+ private static final String PROP_TO = "to";
+ private static final String PROP_CC = "cc";
+ private static final String PROP_BCC = "bcc";
+
+ static final Set PROP_TO_VALUE = new
HashSet<>(Arrays.asList("t...@example.com", "t...@example.org"));
+ static final Set PROP_CC_VALUE = new
HashSet<>(Arrays.asList("cct...@example.com", "cct...@example.org"));
+ static final Set PROP_BCC_VALUE = new
HashSet<>(Arrays.asList("bcct...@example.com", "bcct...@example.org"));
+
+ private static final Map ALERT_PROPS = new HashMap<>();
+
+ static DetectionAlertFilterNotification makeEmailNotifications() {
+return makeEmailNotifications(new HashSet());
+ }
+
+ static DetectionAlertFilterNotification makeEmailNotifications(Set
toRecipients) {
Review comment:
Maybe use this util in the `AlertFilter` classes?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jihaozh commented on a change in pull request #4620: [TE] [Notification] Refactor email filters to a generalized alert filter
jihaozh commented on a change in pull request #4620: [TE] [Notification]
Refactor email filters to a generalized alert filter
URL: https://github.com/apache/incubator-pinot/pull/4620#discussion_r325927989
##
File path:
thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/alert/filter/PerUserDimensionAlertFilter.java
##
@@ -110,13 +110,15 @@ public String apply(MergedAnomalyResultDTO
mergedAnomalyResultDTO) {
}
for (Map.Entry> userAnomalyMapping :
perUserAnomalies.entrySet()) {
- result.addMapping(
- new DetectionAlertFilterRecipients(
- this.makeGroupRecipients(userAnomalyMapping.getKey()),
- this.recipients.get(PROP_CC),
- this.recipients.get(PROP_BCC)),
- new HashSet<>(userAnomalyMapping.getValue())
- );
+ Map> recipients = new HashMap<>();
Review comment:
Maybe put these code into a constructor?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jihaozh commented on a change in pull request #4620: [TE] [Notification] Refactor email filters to a generalized alert filter
jihaozh commented on a change in pull request #4620: [TE] [Notification]
Refactor email filters to a generalized alert filter
URL: https://github.com/apache/incubator-pinot/pull/4620#discussion_r325926448
##
File path:
thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/alert/DetectionAlertFilterNotification.java
##
@@ -0,0 +1,62 @@
+/*
+ * 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.thirdeye.detection.alert;
+
+import java.util.Map;
+import java.util.Objects;
+
+
+/**
+ * Container class for notification properties
+ */
+public class DetectionAlertFilterNotification {
+ Map notificationProps;
+
Review comment:
Maybe add a constructor, for example,
```DetectionAlertFilterNotification(Set to, Set cc, Set
bcc)``` which will create the props map and insert the fields into the props.
This can avoid some code duplication.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] akshayrai commented on a change in pull request #4621: [TE] fix the last time stamp filter out the generated anomalies
akshayrai commented on a change in pull request #4621: [TE] fix the last time
stamp filter out the generated anomalies
URL: https://github.com/apache/incubator-pinot/pull/4621#discussion_r325924451
##
File path:
thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/DetectionResource.java
##
@@ -476,7 +477,11 @@ public Response detectionReplay(
}
for (MergedAnomalyResultDTO anomaly : result.getAnomalies()) {
-anomaly.setAnomalyResultSource(AnomalyResultSource.ANOMALY_REPLAY);
+// if would like to skip sending the alert emails, the anomalies
should be marked with REPLAY
+// otherwise, should be left as the default source
+if (!sendEmail) {
+ anomaly.setAnomalyResultSource(AnomalyResultSource.ANOMALY_REPLAY);
Review comment:
Shouldn't the anomaly source be set to ANOMALY_REPLAY irrespective of the
email?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jihaozh opened a new pull request #4621: [TE] fix the last time stamp filter out the generated anomalies
jihaozh opened a new pull request #4621: [TE] fix the last time stamp filter out the generated anomalies URL: https://github.com/apache/incubator-pinot/pull/4621 - Remove the check that to filter out generated anomalies based on the last timestamp. This may filter out useful anomalies. - In the replay endpoint, add the option to turn on email sending for the generated anomalies. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jihaozh commented on issue #4619: [TE] Add data set display name and show it on UI
jihaozh commented on issue #4619: [TE] Add data set display name and show it on UI URL: https://github.com/apache/incubator-pinot/pull/4619#issuecomment-532874914 > @jihaozh can you provide some context (example might help) behind why we need the display name? Added this per the requirements of UMP alert onboarding. I'll send a follow-up RB to fill the display name for UMP datasets. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] harleyjj commented on issue #4619: [TE] Add data set display name and show it on UI
harleyjj commented on issue #4619: [TE] Add data set display name and show it on UI URL: https://github.com/apache/incubator-pinot/pull/4619#issuecomment-532851936 It looks good, but it's failing CI tests 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] akshayrai commented on issue #4619: [TE] Add data set display name and show it on UI
akshayrai commented on issue #4619: [TE] Add data set display name and show it on UI URL: https://github.com/apache/incubator-pinot/pull/4619#issuecomment-532827199 @jihaozh can you provide some context (example might help) behind why we need the display name? 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] akshayrai opened a new pull request #4620: [TE] [Notification] Refactor email filters to a generalized alert filter
akshayrai opened a new pull request #4620: [TE] [Notification] Refactor email filters to a generalized alert filter URL: https://github.com/apache/incubator-pinot/pull/4620 Alert filter to return a map of (alert-properties, anomalies) rather than just the (recipients, anomalies). This will allow other alerters like jira to pass notification related parameters. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] jihaozh opened a new pull request #4619: [TE] Add data set display name and show it on UI
jihaozh opened a new pull request #4619: [TE] Add data set display name and show it on UI URL: https://github.com/apache/incubator-pinot/pull/4619 - Add the ability to set dataset's display name. If the display name is set, the UI will show the display name instead of the dataset key. - Add dataset and granularity filters in the alert list page. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #4602: First pass of GROUP BY with ORDER BY support
mayankshriv commented on a change in pull request #4602: First pass of GROUP BY
with ORDER BY support
URL: https://github.com/apache/incubator-pinot/pull/4602#discussion_r325800046
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/data/table/ConcurrentIndexedTable.java
##
@@ -19,37 +19,65 @@
package org.apache.pinot.core.data.table;
import com.google.common.base.Preconditions;
+import java.util.ArrayList;
import java.util.Comparator;
import java.util.Iterator;
import java.util.List;
import java.util.PriorityQueue;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.LongAccumulator;
+import java.util.concurrent.atomic.LongAdder;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import javax.annotation.Nonnull;
+import org.apache.commons.collections.CollectionUtils;
import org.apache.pinot.common.request.AggregationInfo;
import org.apache.pinot.common.request.SelectionSort;
import org.apache.pinot.common.utils.DataSchema;
import org.apache.pinot.core.data.order.OrderByUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Thread safe {@link Table} implementation for aggregating TableRecords based
on combination of keys
*/
public class ConcurrentIndexedTable extends IndexedTable {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(ConcurrentIndexedTable.class);
+
private ConcurrentMap _lookupMap;
- private Comparator _minHeapComparator;
private ReentrantReadWriteLock _readWriteLock;
+ private boolean _isOrderBy;
+ private Comparator _minHeapComparator;
+ private Comparator _orderByComparator;
+
+ private AtomicBoolean _noMoreNewRecords = new AtomicBoolean();
+ private LongAdder _numResizes = new LongAdder();
+ private LongAccumulator _resizeTime = new LongAccumulator(Long::sum, 0);
+
+ /**
+ * Initializes the data structures and comparators needed for this Table
+ * @param dataSchema data schema of the record's keys and values
+ * @param aggregationInfos aggregation infors for the aggregations in
record'd values
+ * @param orderBy list of {@link SelectionSort} defining the order by
+ * @param maxCapacity the max number of records to hold
+ * @param sort does final result need to be sorted
+ */
@Override
public void init(@Nonnull DataSchema dataSchema, List
aggregationInfos, List orderBy,
- int maxCapacity) {
-super.init(dataSchema, aggregationInfos, orderBy, maxCapacity);
+ int maxCapacity, boolean sort) {
+super.init(dataSchema, aggregationInfos, orderBy, maxCapacity, sort);
-_minHeapComparator = OrderByUtils.getKeysAndValuesComparator(dataSchema,
orderBy, aggregationInfos).reversed();
_lookupMap = new ConcurrentHashMap<>();
_readWriteLock = new ReentrantReadWriteLock();
+_isOrderBy = CollectionUtils.isNotEmpty(orderBy);
+if (_isOrderBy) {
+ _minHeapComparator = OrderByUtils.getKeysAndValuesComparator(dataSchema,
orderBy, aggregationInfos).reversed();
Review comment:
Will this support order-by with ASC/DESC?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #4602: First pass of GROUP BY with ORDER BY support
mayankshriv commented on a change in pull request #4602: First pass of GROUP BY
with ORDER BY support
URL: https://github.com/apache/incubator-pinot/pull/4602#discussion_r325797789
##
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##
@@ -171,9 +171,12 @@ public ServerType getServerType() {
public static final String SQL = "sql";
public static final String TRACE = "trace";
public static final String DEBUG_OPTIONS = "debugOptions";
+ public static final String QUERY_OPTIONS = "queryOptions";
Review comment:
I don't recall using it for partitioning. But seems this got introduced in:
https://github.com/apache/incubator-pinot/pull/1490
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #4602: First pass of GROUP BY with ORDER BY support
mayankshriv commented on a change in pull request #4602: First pass of GROUP BY
with ORDER BY support
URL: https://github.com/apache/incubator-pinot/pull/4602#discussion_r325807516
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java
##
@@ -392,6 +434,218 @@ private void setAggregationResults(@Nonnull
BrokerResponseNative brokerResponseN
brokerResponseNative.setAggregationResults(reducedAggregationResults);
}
+ /**
+ * Extract group by order by results and set into {@link ResultTable}
+ * @param brokerResponseNative broker response
+ * @param dataSchema data schema
+ * @param aggregationInfos aggregations info
+ * @param groupBy group by info
+ * @param orderBy order by info
+ * @param dataTableMap map from server to data table
+ */
+ private void setSQLGroupByOrderByResults(@Nonnull BrokerResponseNative
brokerResponseNative,
+ @Nonnull DataSchema dataSchema, @Nonnull List
aggregationInfos, @Nonnull GroupBy groupBy,
+ @Nonnull List orderBy, @Nonnull Map dataTableMap,
+ boolean preserveType) {
+
+List columns = new ArrayList<>(dataSchema.size());
+for (int i = 0; i < dataSchema.size(); i++) {
+ columns.add(dataSchema.getColumnName(i));
+}
+
+int numGroupBy = groupBy.getExpressionsSize();
+int numAggregations = aggregationInfos.size();
+
+IndexedTable indexedTable;
+try {
+ indexedTable =
+ getIndexedTable(numGroupBy, numAggregations, groupBy,
aggregationInfos, orderBy, dataSchema, dataTableMap);
+} catch (Throwable throwable) {
+ throw new IllegalStateException(throwable);
+}
+
+List aggregationFunctions = new
ArrayList<>(aggregationInfos.size());
+for (AggregationInfo aggregationInfo : aggregationInfos) {
+ aggregationFunctions.add(
+
AggregationFunctionUtils.getAggregationFunctionContext(aggregationInfo).getAggregationFunction());
+}
+
+List rows = new ArrayList<>();
+int numColumns = columns.size();
+Iterator sortedIterator = indexedTable.iterator();
+int numRows = 0;
+while (numRows < groupBy.getTopN() && sortedIterator.hasNext()) {
+
+ Record nextRecord = sortedIterator.next();
+ Serializable[] row = new Serializable[numColumns];
+ int index = 0;
+ for (Object keyColumn : nextRecord.getKey().getColumns()) {
+row[index ++] = getSerializableValue(keyColumn);
+ }
+ int aggNum = 0;
+ for (Object valueColumn : nextRecord.getValues()) {
+row[index] =
getSerializableValue(aggregationFunctions.get(aggNum).extractFinalResult(valueColumn));
+if (preserveType) {
+ row[index] = AggregationFunctionUtils.formatValue(row[index]);
+}
+index ++;
+ }
+ rows.add(row);
+ numRows++;
+}
+
+brokerResponseNative.setResultTable(new ResultTable(columns, rows));
+ }
+
+ private IndexedTable getIndexedTable(int numGroupBy, int numAggregations,
GroupBy groupBy,
+ List aggregationInfos, List orderBy,
DataSchema dataSchema, Map dataTableMap)
+ throws Throwable {
+
+IndexedTable indexedTable = new ConcurrentIndexedTable();
+// setting a higher value to avoid frequent resizing
+int capacity = (int) Math.max(groupBy.getTopN(), 1);
+indexedTable.init(dataSchema, aggregationInfos, orderBy, capacity, true);
+
+for (DataTable dataTable : dataTableMap.values()) {
+ CheckedFunction2[] functions = new CheckedFunction2[dataSchema.size()];
+ for (int i = 0; i < dataSchema.size(); i++) {
+ColumnDataType columnDataType = dataSchema.getColumnDataType(i);
+CheckedFunction2 function;
+switch (columnDataType) {
+
+ case INT:
+function = (CheckedFunction2)
dataTable::getInt;
+break;
+ case LONG:
+function = (CheckedFunction2)
dataTable::getLong;
+break;
+ case FLOAT:
+function = (CheckedFunction2)
dataTable::getFloat;
+break;
+ case DOUBLE:
+function = (CheckedFunction2)
dataTable::getDouble;
+break;
+ case STRING:
+function = (CheckedFunction2)
dataTable::getString;
+break;
+ default:
+function = (CheckedFunction2)
dataTable::getObject;
+}
+functions[i] = function;
+ }
+
+ for (int row = 0; row < dataTable.getNumberOfRows(); row++) {
+Object[] key = new Object[numGroupBy];
+int col = 0;
+for (int j = 0; j < numGroupBy; j++) {
+ key[j] = functions[col].apply(row, col);
+ col ++;
+}
+Object[] value = new Object[numAggregations];
+for (int j = 0; j < numAggregations; j++) {
+ value[j] = functions[col].apply(row, col);
+ col ++;
+}
+Record record = new Record(new Key(key), value);
+indexedTable.upsert(record);
+ }
+}
+
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #4602: First pass of GROUP BY with ORDER BY support
mayankshriv commented on a change in pull request #4602: First pass of GROUP BY
with ORDER BY support
URL: https://github.com/apache/incubator-pinot/pull/4602#discussion_r325795302
##
File path:
pinot-common/src/main/java/org/apache/pinot/common/response/broker/ResultTable.java
##
@@ -0,0 +1,62 @@
+/**
+ * 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.common.response.broker;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonPropertyOrder;
+import java.io.Serializable;
+import java.util.List;
+
+
+/**
+ * Hosts the results in a standard tabular structure
+ */
+@JsonPropertyOrder({"columns", "results"})
Review comment:
Nit: why not s/results/rows, especially when the member variable is called
_rows?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #4602: First pass of GROUP BY with ORDER BY support
mayankshriv commented on a change in pull request #4602: First pass of GROUP BY
with ORDER BY support
URL: https://github.com/apache/incubator-pinot/pull/4602#discussion_r325806695
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java
##
@@ -246,22 +264,46 @@ public BrokerResponseNative reduceOnDataTable(@Nonnull
BrokerRequest brokerReque
// Aggregation query.
AggregationFunction[] aggregationFunctions =
AggregationFunctionUtils.getAggregationFunctions(brokerRequest.getAggregationsInfo());
+
if (!brokerRequest.isSetGroupBy()) {
// Aggregation only query.
setAggregationResults(brokerResponseNative, aggregationFunctions,
dataTableMap, cachedDataSchema,
preserveType);
-} else {
- // Aggregation group-by query.
- boolean[] aggregationFunctionSelectStatus =
-
AggregationFunctionUtils.getAggregationFunctionsSelectStatus(brokerRequest.getAggregationsInfo());
- setGroupByHavingResults(brokerResponseNative, aggregationFunctions,
aggregationFunctionSelectStatus,
- brokerRequest.getGroupBy(), dataTableMap,
brokerRequest.getHavingFilterQuery(),
- brokerRequest.getHavingFilterSubQueryMap(), preserveType);
- if (brokerMetrics != null &&
(!brokerResponseNative.getAggregationResults().isEmpty())) {
-// We emit the group by size when the result isn't empty. All the
sizes among group-by results should be the same.
-// Thus, we can just emit the one from the 1st result.
-brokerMetrics.addMeteredQueryValue(brokerRequest,
BrokerMeter.GROUP_BY_SIZE,
-
brokerResponseNative.getAggregationResults().get(0).getGroupByResult().size());
+} else { // Aggregation group-by query.
Review comment:
May be factorize this as a utility function?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #4602: First pass of GROUP BY with ORDER BY support
mayankshriv commented on a change in pull request #4602: First pass of GROUP BY with ORDER BY support URL: https://github.com/apache/incubator-pinot/pull/4602#discussion_r325805705 ## File path: pinot-core/src/main/java/org/apache/pinot/core/operator/query/AggregationGroupByOperator.java ## @@ -58,6 +64,34 @@ public AggregationGroupByOperator(@Nonnull AggregationFunctionContext[] function _transformOperator = transformOperator; _numTotalRawDocs = numTotalRawDocs; _useStarTree = useStarTree; + +int numColumns = groupBy.getExpressionsSize() + _functionContexts.length; Review comment: Seems like this is a change in the default path, purely due to signature change for constructor of IntermediateResultBlock? Ideally, I'd want to have no changes to default path while this project is still on-going. Of course, we can take smart risks depending on how the change. I'll let you assess that for this case. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #4602: First pass of GROUP BY with ORDER BY support
mayankshriv commented on a change in pull request #4602: First pass of GROUP BY
with ORDER BY support
URL: https://github.com/apache/incubator-pinot/pull/4602#discussion_r325804508
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java
##
@@ -204,18 +215,25 @@ public BrokerResponseNative reduceOnDataTable(@Nonnull
BrokerRequest brokerReque
}
// Parse the option from request whether to preserve the type
-String preserveTypeString = (brokerRequest.getQueryOptions() == null) ?
"false" : brokerRequest.getQueryOptions()
-
.getOrDefault(CommonConstants.Broker.Request.QueryOptionKey.PRESERVE_TYPE,
"false");
+Map queryOptions = brokerRequest.getQueryOptions();
+String preserveTypeString =
+(queryOptions == null) ? "false" :
queryOptions.getOrDefault(QueryOptionKey.PRESERVE_TYPE, "false");
boolean preserveType = Boolean.valueOf(preserveTypeString);
if (dataTableMap.isEmpty()) {
- // For empty data table map, construct empty result using the cached
data schema.
-
- // This will only happen to selection query.
+ // even though results empty, set columns for these operations
if (cachedDataSchema != null) {
-List selectionColumns = SelectionOperatorUtils
-
.getSelectionColumns(brokerRequest.getSelections().getSelectionColumns(),
cachedDataSchema);
-brokerResponseNative.setSelectionResults(new
SelectionResults(selectionColumns, new ArrayList<>(0)));
+if (brokerRequest.isSetSelections()) {
+ List selectionColumns =
+
SelectionOperatorUtils.getSelectionColumns(brokerRequest.getSelections().getSelectionColumns(),
+ cachedDataSchema);
+ brokerResponseNative.setSelectionResults(new
SelectionResults(selectionColumns, new ArrayList<>(0)));
+} else if (brokerRequest.isSetGroupBy() && queryOptions != null &&
SQL.equalsIgnoreCase(
+queryOptions.get(QueryOptionKey.GROUP_BY_MODE)) &&
SQL.equalsIgnoreCase(
+queryOptions.get(QueryOptionKey.RESPONSE_FORMAT))) {
Review comment:
Is RESPONSE_FORMAT something that user specifies? If so, are we support
query type SQL and response type PQL, and other such combinations? Not sure how
that would useful, why not just dictate response based on query type (in which
case we don't need this extra option)?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on issue #4608: Unit tests and bug fixes for DeleteTable rest API for controller.
codecov-io commented on issue #4608: Unit tests and bug fixes for DeleteTable rest API for controller. URL: https://github.com/apache/incubator-pinot/pull/4608#issuecomment-532548562 # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4608?src=pr=h1) Report > Merging [#4608](https://codecov.io/gh/apache/incubator-pinot/pull/4608?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/fe7fff6288464aaf430ab850741000481fa704fc?src=pr=desc) will **increase** coverage by `0.04%`. > The diff coverage is `100%`. [](https://codecov.io/gh/apache/incubator-pinot/pull/4608?src=pr=tree) ```diff @@ Coverage Diff @@ ## master#4608 +/- ## + Coverage 64.33% 64.37% +0.04% + Complexity 94 32 -62 Files 1072 1072 Lines 5570255710 +8 Branches 8131 8137 +6 + Hits 3583635865 +29 + Misses1721617203 -13 + Partials 2650 2642 -8 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/4608?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...oller/api/resources/PinotTableRestletResource.java](https://codecov.io/gh/apache/incubator-pinot/pull/4608/diff?src=pr=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFibGVSZXN0bGV0UmVzb3VyY2UuamF2YQ==) | `57.35% <100%> (+4.8%)` | `0 <0> (ø)` | :arrow_down: | | [.../org/apache/pinot/client/PinotClientException.java](https://codecov.io/gh/apache/incubator-pinot/pull/4608/diff?src=pr=tree#diff-cGlub3QtYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jbGllbnQvUGlub3RDbGllbnRFeGNlcHRpb24uamF2YQ==) | `33.33% <0%> (-33.34%)` | `0% <0%> (-2%)` | | | [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/4608/diff?src=pr=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `23.8% <0%> (-19.05%)` | `0% <0%> (ø)` | | | [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/4608/diff?src=pr=tree#diff-cGlub3QtYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jbGllbnQvSnNvbkFzeW5jSHR0cFBpbm90Q2xpZW50VHJhbnNwb3J0LmphdmE=) | `58.69% <0%> (-15.22%)` | `0% <0%> (-5%)` | | | [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/4608/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `65.21% <0%> (-10.87%)` | `0% <0%> (ø)` | | | [...mpl/dictionary/DoubleOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/4608/diff?src=pr=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT2ZmSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `73.21% <0%> (-5.36%)` | `0% <0%> (ø)` | | | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/4608/diff?src=pr=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `74.16% <0%> (-3.34%)` | `0% <0%> (ø)` | | | [.../org/apache/pinot/transport/netty/NettyServer.java](https://codecov.io/gh/apache/incubator-pinot/pull/4608/diff?src=pr=tree#diff-cGlub3QtdHJhbnNwb3J0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC90cmFuc3BvcnQvbmV0dHkvTmV0dHlTZXJ2ZXIuamF2YQ==) | `80.8% <0%> (-3.04%)` | `0% <0%> (ø)` | | | [...not/broker/broker/helix/ClusterChangeMediator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4608/diff?src=pr=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0NsdXN0ZXJDaGFuZ2VNZWRpYXRvci5qYXZh) | `76.92% <0%> (-2.2%)` | `0% <0%> (ø)` | | | [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/4608/diff?src=pr=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | `53.29% <0%> (-1.8%)` | `0% <0%> (ø)` | | | ... and [21 more](https://codecov.io/gh/apache/incubator-pinot/pull/4608/diff?src=pr=tree-more) | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4608?src=pr=continue). > **Legend** - [Click here to learn
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.
chenboat commented on a change in pull request #4608: Unit tests and bug fixes
for DeleteTable rest API for controller.
URL: https://github.com/apache/incubator-pinot/pull/4608#discussion_r325495667
##
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java
##
@@ -273,6 +275,76 @@ public void rebalanceTableWithoutSegments()
Assert.assertEquals(rebalanceResult.getStatus(),
RebalanceResult.Status.NO_OP);
}
+ @Test
+ public void testDeleteTable() throws IOException {
+// Case 1: Create a REALTIME table and delete it directly w/o using query
param.
+TableConfig realtimeTableConfig =
_realtimeBuilder.setTableName("table0").build();
+String creationResponse =
+sendPostRequest(_createTableUrl,
realtimeTableConfig.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table0_REALTIME
succesfully added\"}");
+
+// Delete realtime table using REALTIME suffix.
+String deleteResponse = sendDeleteRequest(StringUtil.join("/",
this._controllerBaseApiUrl,
+"tables", "testRealtimeTable_REALTIME"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables:
[testRealtimeTable_REALTIME] deleted\"}");
+
+// Case 2: Create an offline table and delete it directly w/o using query
param.
+TableConfig offlineTableConfig =
_offlineBuilder.setTableName("table0").build();
+creationResponse =
+sendPostRequest(_createTableUrl,
offlineTableConfig.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table0_OFFLINE
succesfully added\"}");
+
+// Delete offline table using OFFLINE suffix.
+deleteResponse = sendDeleteRequest(StringUtil.join("/",
this._controllerBaseApiUrl,
+"tables", "table0_OFFLINE"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables:
[table0_OFFLINE] deleted\"}");
+
+// Case 3: Create REALTIME and OFFLINE tables and delete both of them.
+TableConfig rtConfig1 = _realtimeBuilder.setTableName("table1").build();
+creationResponse =
+sendPostRequest(_createTableUrl, rtConfig1.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table1_REALTIME
succesfully added\"}");
+
+TableConfig offlineConfig1 =
_offlineBuilder.setTableName("table1").build();
+creationResponse =
+sendPostRequest(_createTableUrl, offlineConfig1.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table1_OFFLINE
succesfully added\"}");
+
+deleteResponse = sendDeleteRequest(StringUtil.join("/",
this._controllerBaseApiUrl,
+"tables", "table1"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables:
[table1_OFFLINE, table1_REALTIME] deleted\"}");
+
+
+// Case 4: Create REALTIME and OFFLINE tables and delete the
realtime/offline table using query params.
+TableConfig rtConfig2 = _realtimeBuilder.setTableName("table2").build();
+creationResponse =
+sendPostRequest(_createTableUrl, rtConfig2.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table2_REALTIME
succesfully added\"}");
+
+TableConfig offlineConfig2 =
_offlineBuilder.setTableName("table2").build();
+creationResponse =
+sendPostRequest(_createTableUrl, offlineConfig2.toJsonConfigString());
+Assert.assertEquals(creationResponse, "{\"status\":\"Table table2_OFFLINE
succesfully added\"}");
+
+deleteResponse = sendDeleteRequest(StringUtil.join("/",
this._controllerBaseApiUrl,
+"tables", "table2?type=realtime"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables:
[table2_REALTIME] deleted\"}");
+
+deleteResponse = sendDeleteRequest(StringUtil.join("/",
this._controllerBaseApiUrl,
+"tables", "table2?type=offline"));
+Assert.assertEquals(deleteResponse, "{\"status\":\"Tables:
[table2_OFFLINE] deleted\"}");
+
+// Case 5: Delete a non-existent table and expect a bad request expection.
+try {
+ deleteResponse = sendDeleteRequest(
+ StringUtil.join("/", this._controllerBaseApiUrl, "tables",
"no_such_table_OFFLINE"));
Review comment:
Done.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.
chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller. URL: https://github.com/apache/incubator-pinot/pull/4608#discussion_r325495647 ## File path: pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java ## @@ -21,10 +21,12 @@ import com.fasterxml.jackson.databind.JsonNode; import java.io.FileNotFoundException; import java.io.IOException; + Review comment: Done. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #4608: Unit tests and bug fixes for DeleteTable rest API for controller.
chenboat commented on a change in pull request #4608: Unit tests and bug fixes
for DeleteTable rest API for controller.
URL: https://github.com/apache/incubator-pinot/pull/4608#discussion_r325495546
##
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
##
@@ -254,20 +254,41 @@ public SuccessResponse deleteTable(
List tablesDeleted = new LinkedList<>();
try {
- if (tableType != TableType.REALTIME &&
!TableNameBuilder.REALTIME.tableHasTypeSuffix(tableName)) {
+ if (toDeleteOfflineTable(tableName, tableType) &&
_pinotHelixResourceManager
+ .hasOfflineTable(tableName)) {
_pinotHelixResourceManager.deleteOfflineTable(tableName);
tablesDeleted.add(TableNameBuilder.OFFLINE.tableNameWithType(tableName));
}
- if (tableType != TableType.OFFLINE &&
!TableNameBuilder.OFFLINE.tableHasTypeSuffix(tableName)) {
+ if (toDeleteRealtimeTable(tableName, tableType) &&
_pinotHelixResourceManager
+ .hasRealtimeTable(tableName)) {
_pinotHelixResourceManager.deleteRealtimeTable(tableName);
tablesDeleted.add(TableNameBuilder.REALTIME.tableNameWithType(tableName));
}
+ if (tablesDeleted.size() == 0) {
+throw new ControllerApplicationException(LOGGER, "Table '" + tableName
+ "' does not exist",
+Response.Status.NOT_FOUND);
+ }
return new SuccessResponse("Tables: " + tablesDeleted + " deleted");
} catch (Exception e) {
- throw new ControllerApplicationException(LOGGER, e.getMessage(),
Response.Status.INTERNAL_SERVER_ERROR, e);
+ throw new ControllerApplicationException(LOGGER, e.getMessage(),
+ Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+ // Return true if tableType is NOT offline (i.e., null or realtime) and
table name does not end
+ // with _offline suffix.
+ private boolean toDeleteRealtimeTable(String tableName, TableType tableType)
{
Review comment:
Done. Combine two functions into one: I also use positive condition testing
to make it easier to understand.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
