You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/04/09 01:19:42 UTC
[GitHub] [druid] vogievetsky opened a new pull request #11085: Web console: Do not put __time in the dimensions list
vogievetsky opened a new pull request #11085:
URL: https://github.com/apache/druid/pull/11085
A change made in https://github.com/apache/druid/pull/10267 to forbid `__time` in the dimensions list has exposed the fact that the transform step of the data loader did just that.
--
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #11085: Web console: Do not put __time in the dimensions list
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #11085:
URL: https://github.com/apache/druid/pull/11085#discussion_r611250604
##########
File path: web-console/e2e-tests/tutorial-batch.spec.ts
##########
@@ -195,6 +191,7 @@ async function validateQuery(page: playwright.Page, datasourceName: string) {
/* page */ 'Talk:Oswald Tilghman',
/* regionIsoCode */ 'null',
/* regionName */ 'null',
+ /* time */ '2015-09-12T00:46:58.771Z',
Review comment:
I updated the e2e test and realized/was faced with this interesting side-effect:
If you specify the time column via the timestampSpec then that column gets excluded from the dimension auto-detection system. If instead you use a transform to construct a `__time` dimension then (obviously) the component dimensions do not get removed from the auto detection. In this case I set `__time` to be `timestamp_parse("time") + 1` and now clicking though all the defaults yields an extra field, the original time column.
--
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11085: Web console: Do not put __time in the dimensions list
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11085:
URL: https://github.com/apache/druid/pull/11085#discussion_r610263116
##########
File path: web-console/src/druid-models/transform-spec.spec.ts
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.
+ */
+
+import { getDimensionNamesFromTransforms } from './transform-spec';
+
+describe('transform-spec', () => {
+ describe('getDimensionNamesFromTransforms', () => {
+ it('does not return the __time column', () => {
+ expect(
+ getDimensionNamesFromTransforms([
+ {
+ type: 'expression',
+ name: 'fooPage',
+ expression: "concat('foo' + page)",
+ },
+ {
+ name: '__time',
+ type: 'expression',
+ expression: "timestamp_shift(timestamp_parse(timestamp), 'P3Y', 1)",
+ },
+ {
+ type: 'expression',
+ name: 'barPage',
+ expression: "concat('foo' + page)",
Review comment:
heh, `barPage` concats `'foo'`?
--
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #11085: Web console: Do not put __time in the dimensions list
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #11085:
URL: https://github.com/apache/druid/pull/11085#issuecomment-816801754
Could you add an end-to-end test too? Manipulations of `__time` during ingest seem like a potentially tricky area in general, and an end to end test would be helpful to make sure it always keeps working in the future.
--
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11085: Web console: Do not put __time in the dimensions list
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11085:
URL: https://github.com/apache/druid/pull/11085#discussion_r610322857
##########
File path: web-console/src/druid-models/transform-spec.spec.ts
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.
+ */
+
+import { getDimensionNamesFromTransforms } from './transform-spec';
+
+describe('transform-spec', () => {
+ describe('getDimensionNamesFromTransforms', () => {
+ it('does not return the __time column', () => {
+ expect(
+ getDimensionNamesFromTransforms([
+ {
+ type: 'expression',
+ name: 'fooPage',
+ expression: "concat('foo' + page)",
+ },
+ {
+ name: '__time',
+ type: 'expression',
+ expression: "timestamp_shift(timestamp_parse(timestamp), 'P3Y', 1)",
+ },
+ {
+ type: 'expression',
+ name: 'barPage',
+ expression: "concat('foo' + page)",
Review comment:
good catch! we don't want our Druid bits to get too corroded from any water spillover into the query request packets, else we might one day wake up to find all our code has been converted to rust.
--
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on pull request #11085: Web console: Do not put __time in the dimensions list
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on pull request #11085:
URL: https://github.com/apache/druid/pull/11085#issuecomment-817087910
I'll see what I can do
--
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #11085: Web console: Do not put __time in the dimensions list
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #11085:
URL: https://github.com/apache/druid/pull/11085#discussion_r610317625
##########
File path: web-console/src/druid-models/transform-spec.spec.ts
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.
+ */
+
+import { getDimensionNamesFromTransforms } from './transform-spec';
+
+describe('transform-spec', () => {
+ describe('getDimensionNamesFromTransforms', () => {
+ it('does not return the __time column', () => {
+ expect(
+ getDimensionNamesFromTransforms([
+ {
+ type: 'expression',
+ name: 'fooPage',
+ expression: "concat('foo' + page)",
+ },
+ {
+ name: '__time',
+ type: 'expression',
+ expression: "timestamp_shift(timestamp_parse(timestamp), 'P3Y', 1)",
+ },
+ {
+ type: 'expression',
+ name: 'barPage',
+ expression: "concat('foo' + page)",
Review comment:
This test is also verifying that running this function on transforms that have the same expression does not does not cause an asynchronous desynchronization of the DOM rendering thread with the memory management subsystem thereby causing memory liquefaction, which will cause a [memory leek](https://user-images.githubusercontent.com/177816/114123947-35d4d380-98a8-11eb-8379-e6b1baf4c9bd.png) and water damage the browser.
--
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky merged pull request #11085: Web console: Do not put __time in the dimensions list
Posted by GitBox <gi...@apache.org>.
vogievetsky merged pull request #11085:
URL: https://github.com/apache/druid/pull/11085
--
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #11085: Web console: Do not put __time in the dimensions list
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #11085:
URL: https://github.com/apache/druid/pull/11085#discussion_r611695827
##########
File path: web-console/e2e-tests/tutorial-batch.spec.ts
##########
@@ -195,6 +191,7 @@ async function validateQuery(page: playwright.Page, datasourceName: string) {
/* page */ 'Talk:Oswald Tilghman',
/* regionIsoCode */ 'null',
/* regionName */ 'null',
+ /* time */ '2015-09-12T00:46:58.771Z',
Review comment:
Does this cause a problem? It's documented behavior (item 3): https://druid.apache.org/docs/latest/ingestion/index.html#inclusions-and-exclusions
--
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #11085: Web console: Do not put __time in the dimensions list
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #11085:
URL: https://github.com/apache/druid/pull/11085#discussion_r611793779
##########
File path: web-console/e2e-tests/tutorial-batch.spec.ts
##########
@@ -195,6 +191,7 @@ async function validateQuery(page: playwright.Page, datasourceName: string) {
/* page */ 'Talk:Oswald Tilghman',
/* regionIsoCode */ 'null',
/* regionName */ 'null',
+ /* time */ '2015-09-12T00:46:58.771Z',
Review comment:
No, it is not a problem. Just something that took me by surprise. Simply because I did not put 1 + 1 together. Maybe there should be some docs of this inside the web console.
--
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #11085: Web console: Do not put __time in the dimensions list
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #11085:
URL: https://github.com/apache/druid/pull/11085#discussion_r610759119
##########
File path: web-console/src/druid-models/transform-spec.spec.ts
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.
+ */
+
+import { getDimensionNamesFromTransforms } from './transform-spec';
+
+describe('transform-spec', () => {
+ describe('getDimensionNamesFromTransforms', () => {
+ it('does not return the __time column', () => {
+ expect(
+ getDimensionNamesFromTransforms([
+ {
+ type: 'expression',
+ name: 'fooPage',
+ expression: "concat('foo' + page)",
+ },
+ {
+ name: '__time',
+ type: 'expression',
+ expression: "timestamp_shift(timestamp_parse(timestamp), 'P3Y', 1)",
+ },
+ {
+ type: 'expression',
+ name: 'barPage',
+ expression: "concat('foo' + page)",
Review comment:
😐
--
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org