feat(db engines): add support for Opendistro Elasticsearch (AWS ES) by dpgaspar · Pull Request #12602 · apache/superset

dpgaspar · 2021-01-19T15:26:54Z

SUMMARY

Adds Elasticsearch opendistro support. Most work was done upstream on the release of elasticsearch-dbapi 0.2.0.
Checkout https://github.com/preset-io/elasticsearch-dbapi/blob/master/README.md for more details.

ADDITIONAL INFORMATION

villebro

Awesome! Quick first pass review

villebro · 2021-01-19T15:33:49Z

+    def make_label_compatible(cls, label: str) -> str:
+        new_label = super().make_label_compatible(label)
+        return new_label.replace(".", "_")


Instead of overriding make_label_compatible, we should define a custom _mutate_label method that does this. See e.g.

superset/superset/db_engine_specs/bigquery.py

Lines 113 to 135 in 9771b82

@staticmethod

def _mutate_label(label: str) -> str:

"""

BigQuery field_name should start with a letter or underscore and contain only

alphanumeric characters. Labels that start with a number are prefixed with an

underscore. Any unsupported characters are replaced with underscores and an

md5 hash is added to the end of the label to avoid possible collisions.

:param label: Expected expression label

:return: Conditionally mutated label

"""

label_hashed = "_" + hashlib.md5(label.encode("utf-8")).hexdigest()

# if label starts with number, add underscore as first character

label_mutated = "_" + label if re.match(r"^\d", label) else label

# replace non-alphanumeric characters with underscores

label_mutated = re.sub(r"[^\w]+", "_", label_mutated)

if label_mutated != label:

# add first 5 chars from md5 hash to label to avoid possible collisions

label_mutated += label_hashed[:6]

return label_mutated

thks! I guess we should rename _mutate_label to mutate_label. This is still on it's infancy, still missing the dbapi PR merge and release and on this side the time grains are different for opendistro

Agree on renaming (let's have a separate PR for that). I guess we should have a separate spec for elasticsearch and odelasticsearch to properly support the differing time grains?

codecov-io · 2021-01-19T16:34:43Z

Codecov Report

Merging #12602 (83df757) into master (bab86ab) will increase coverage by 2.31%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master   #12602      +/-   ##
==========================================
+ Coverage   65.05%   67.36%   +2.31%     
==========================================
  Files        1021      489     -532     
  Lines       50095    28772   -21323     
  Branches     5141        0    -5141     
==========================================
- Hits        32587    19383   -13204     
+ Misses      17332     9389    -7943     
+ Partials      176        0     -176

Flag	Coverage Δ
cypress	`?`
javascript	`?`
python	`67.36% <80.00%> (+3.29%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/db_engine_specs/elasticsearch.py	`89.74% <80.00%> (-4.71%)`	⬇️
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/db_engine_specs/hive.py	`73.84% <0.00%> (-17.31%)`	⬇️
superset/dataframe.py	`91.66% <0.00%> (-8.34%)`	⬇️
superset/db_engine_specs/clickhouse.py	`87.09% <0.00%> (-7.35%)`	⬇️
superset/db_engine_specs/presto.py	`81.38% <0.00%> (-6.71%)`	⬇️
superset/dashboards/commands/update.py	`83.07% <0.00%> (-5.61%)`	⬇️
superset/utils/decorators.py	`94.44% <0.00%> (-5.56%)`	⬇️
superset/views/database/mixins.py	`80.70% <0.00%> (-1.76%)`	⬇️
... and 565 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bab86ab...83df757. Read the comment docs.

syamat · 2021-01-24T14:50:23Z

When is the plan to merge this PR? Any time lines?

dpgaspar · 2021-01-26T09:35:24Z

@syamat this still needs some work on the dialect side also https://github.com/preset-io/elasticsearch-dbapi, but hopping to have this merged in 2 weeks

villebro

One quick question/comment

villebro · 2021-02-09T10:09:27Z

+    @classmethod
+    def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
+        if target_type.upper() == utils.TemporalType.DATETIME:
+            return f"""'{dttm.isoformat(timespec="seconds")}'"""
+        return None


Doesn't ES/OD support other temporal types like DATE or TIMESTAMP'?

To my understanding by default Elasticsearch only supports date field type

https://opendistro.github.io/for-elasticsearch-docs/docs/sql/datatypes/#date-and-time-types

... By default, the Elasticsearch DSL uses the date type as the only date-time related type that contains all information of an absolute time point. ...

For the SQL endpoint, we can use date functions that will return other date/time types out of them

Ok thanks for clarifying!

villebro

LGTM!

* master: fix: UI toast typo (apache#13026) feat(db engines): add support for Opendistro Elasticsearch (AWS ES) (apache#12602) fix(build): black failing on master, add to required checks (apache#13039) fix: time filter db migration optimization (apache#13015) fix: untranslated text content of Dashboard page (apache#13024) fix(ci): remove signature requirements for commits to master (apache#13034) fix: add alerts and report to default config (apache#12999) docs(changelog): add entries for 1.0.1 (apache#12981) ci: skip cypress if no code changes (apache#12982) chore: add cypress required checks for branch protection (apache#12970) Refresh dashboard list after bulk delete (apache#12945) Updates storybook to version 6.1.17 (apache#13014) feat: Save datapanel state in local storage (apache#12996) fix: added text and changed margins (apache#12923) chore: Swap Slack Url 2 more places (apache#13004)

…pache#12602) * feat(db engines): add support for Opendistro Elasticsearch (AWS ES) * add time grains * lint * bump elasticsearch-dbapi version * add tests * fix test

Jimmy-Newtron · 2021-02-15T21:09:38Z

@villebro When do you plan to release a new version with this improvement ?

…pache#12602) * feat(db engines): add support for Opendistro Elasticsearch (AWS ES) * add time grains * lint * bump elasticsearch-dbapi version * add tests * fix test (cherry picked from commit b3a814f)

vineetalagh · 2021-05-17T12:08:13Z

@dpgaspar Please look at issue 14662. I think the conversion for opendistro engine is not working.

…pache#12602) * feat(db engines): add support for Opendistro Elasticsearch (AWS ES) * add time grains * lint * bump elasticsearch-dbapi version * add tests * fix test

feat(db engines): add support for Opendistro Elasticsearch (AWS ES)

5f3821c

superset-github-bot Bot added the preset-io label Jan 19, 2021

pull-request-size Bot added the size/S label Jan 19, 2021

villebro reviewed Jan 19, 2021

View reviewed changes

dpgaspar added 2 commits January 21, 2021 12:30

add time grains

e664cab

lint

52b049b

pull-request-size Bot added size/M and removed size/S labels Jan 21, 2021

dpgaspar added 2 commits January 29, 2021 10:34

Merge branch 'master' into feat/opendistro

4f1fc06

bump elasticsearch-dbapi version

84f9000

dpgaspar marked this pull request as ready for review February 8, 2021 14:01

add tests

57773ef

dpgaspar requested a review from villebro February 8, 2021 14:25

fix test

83df757

villebro reviewed Feb 9, 2021

View reviewed changes

villebro approved these changes Feb 9, 2021

View reviewed changes

dpgaspar merged commit b3a814f into apache:master Feb 10, 2021

dpgaspar deleted the feat/opendistro branch February 10, 2021 08:17

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.0 labels Mar 12, 2024

	@staticmethod
	def _mutate_label(label: str) -> str:
	"""
	BigQuery field_name should start with a letter or underscore and contain only
	alphanumeric characters. Labels that start with a number are prefixed with an
	underscore. Any unsupported characters are replaced with underscores and an
	md5 hash is added to the end of the label to avoid possible collisions.

	:param label: Expected expression label
	:return: Conditionally mutated label
	"""
	label_hashed = "_" + hashlib.md5(label.encode("utf-8")).hexdigest()

	# if label starts with number, add underscore as first character
	label_mutated = "_" + label if re.match(r"^\d", label) else label

	# replace non-alphanumeric characters with underscores
	label_mutated = re.sub(r"[^\w]+", "_", label_mutated)
	if label_mutated != label:
	# add first 5 chars from md5 hash to label to avoid possible collisions
	label_mutated += label_hashed[:6]

	return label_mutated

Uh oh!

Conversation

dpgaspar commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

ADDITIONAL INFORMATION

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

villebro Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

dpgaspar Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

villebro Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

syamat commented Jan 24, 2021

Uh oh!

dpgaspar commented Jan 26, 2021

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

villebro Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

dpgaspar Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

villebro Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

Jimmy-Newtron commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vineetalagh commented May 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dpgaspar commented Jan 19, 2021 •

edited

Loading

codecov-io commented Jan 19, 2021 •

edited

Loading

Jimmy-Newtron commented Feb 15, 2021 •

edited

Loading