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 2021/02/01 19:07:31 UTC
[GitHub] [incubator-pinot] tejasajmera opened a new pull request #6514: [TE]frontend - Add filtering support for Entity Monitoring tables
tejasajmera opened a new pull request #6514:
URL: https://github.com/apache/incubator-pinot/pull/6514
Filtering is supposed to work on column level with all possible data types in the table. It will do a substring match of the string passed in with each of the column entries; a return value of true would make table display the corresponding row.
Miscellaneous
1. Refactored `anomaliesDetails` to avoid doing the O(N) computation along with O(N) memory involved in converting object to array to be used in the template. Instead the template is modified to directly work with the object.
2. Refactored the columns in each of the table for entity monitoring to ensure we columns which can be shared.
Note to reviewers:
Lot of changes in `utils.js` are style related auto changed by the prettifier. Please focus on just 2 functions in that file -`isObject` and `checkForMatch`. Both are towards the end of the file.
----------------------------------------------------------------
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] zhangloo333 commented on a change in pull request #6514: [TE]frontend - Add filtering support for Entity Monitoring tables
Posted by GitBox <gi...@apache.org>.
zhangloo333 commented on a change in pull request #6514:
URL: https://github.com/apache/incubator-pinot/pull/6514#discussion_r568086902
##########
File path: thirdeye/thirdeye-frontend/app/utils/utils.js
##########
@@ -275,11 +277,68 @@ export function buildBounds(series, baseline, timeseries, useCurrent) {
* @param {Array} timeSeries - time series to modify
*/
export function stripNonFiniteValues(timeSeries) {
- return timeSeries.map(value => {
- return (isFinite(value) ? value : null);
+ return timeSeries.map((value) => {
+ return isFinite(value) ? value : null;
});
}
+/*
+ * Detect if the input is in Object form
+ *
+ * @param {Any} input
+ * The input to test.
+ *
+ * @returns {Boolean}
+ * True if input is an object, false otherwise.
+ */
+export function isObject(input) {
+ return input instanceof Object && input.constructor === Object;
+}
+
+/*
+ * Search the input for the existence of the search term
+ * -Supports searching on following input types - string, integer, float, boolean, object, array
+ *
+ * @param {Any} input
+ * The input to test.
+ * @param {String} filterStr
+ * The search term
+ *
+ * @returns {Boolean}
+ * True if the match is found, false otherwise.
+ */
+export function checkForMatch(input, filterStr) {
+ if (filterStr === '') {
+ return true;
+ }
+
+ if (typeof input === 'string') {
+ return input.includes(filterStr);
+ } else if (typeof input === 'number' || typeof input === 'boolean') {
+ return input.toString().includes(filterStr);
+ } else if (isObject(input)) {
+ for (const prop in input) {
+ if ({}.propertyIsEnumerable.call(input, prop)) {
+ if (checkForMatch(prop, filterStr) || checkForMatch(input[prop], filterStr)) {
Review comment:
multiple level nest conditions will muddy up code's control flow. Also, it takes difficulty for later maintenance and result devs to spend more time on understanding state and logic. These two `if` statement are unnesstary, I suggest u can put them in one such as `if ({}.propertyIsEnumerable.call(input, prop) && (checkForMatch(prop, filterStr) || checkForMatch(input[prop], filterStr))`
----------------------------------------------------------------
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] tejasajmera commented on a change in pull request #6514: [TE]frontend - Add filtering support for Entity Monitoring tables
Posted by GitBox <gi...@apache.org>.
tejasajmera commented on a change in pull request #6514:
URL: https://github.com/apache/incubator-pinot/pull/6514#discussion_r568099786
##########
File path: thirdeye/thirdeye-frontend/app/utils/utils.js
##########
@@ -275,11 +277,68 @@ export function buildBounds(series, baseline, timeseries, useCurrent) {
* @param {Array} timeSeries - time series to modify
*/
export function stripNonFiniteValues(timeSeries) {
- return timeSeries.map(value => {
- return (isFinite(value) ? value : null);
+ return timeSeries.map((value) => {
+ return isFinite(value) ? value : null;
});
}
+/*
+ * Detect if the input is in Object form
+ *
+ * @param {Any} input
+ * The input to test.
+ *
+ * @returns {Boolean}
+ * True if input is an object, false otherwise.
+ */
+export function isObject(input) {
+ return input instanceof Object && input.constructor === Object;
+}
+
+/*
+ * Search the input for the existence of the search term
+ * -Supports searching on following input types - string, integer, float, boolean, object, array
+ *
+ * @param {Any} input
+ * The input to test.
+ * @param {String} filterStr
+ * The search term
+ *
+ * @returns {Boolean}
+ * True if the match is found, false otherwise.
+ */
+export function checkForMatch(input, filterStr) {
+ if (filterStr === '') {
+ return true;
+ }
+
+ if (typeof input === 'string') {
+ return input.includes(filterStr);
+ } else if (typeof input === 'number' || typeof input === 'boolean') {
+ return input.toString().includes(filterStr);
+ } else if (isObject(input)) {
+ for (const prop in input) {
+ if ({}.propertyIsEnumerable.call(input, prop)) {
+ if (checkForMatch(prop, filterStr) || checkForMatch(input[prop], filterStr)) {
Review comment:
I can club the `if's` but I don't necessarily agree it leads to improved maintenance or less time understanding the logic, as the logic is not really changed by clubbing the `if's`.
----------------------------------------------------------------
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] jihaozh merged pull request #6514: [TE]frontend - Add filtering support for Entity Monitoring tables
Posted by GitBox <gi...@apache.org>.
jihaozh merged pull request #6514:
URL: https://github.com/apache/incubator-pinot/pull/6514
----------------------------------------------------------------
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] jihaozh merged pull request #6514: [TE]frontend - Add filtering support for Entity Monitoring tables
Posted by GitBox <gi...@apache.org>.
jihaozh merged pull request #6514:
URL: https://github.com/apache/incubator-pinot/pull/6514
----------------------------------------------------------------
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