Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(search): Add search for field level description and tags #2491

Merged

Conversation

dexter-mh-lee
Copy link
Contributor

Add search for field level description and tags. Also add search for dataset description.

For field-level description and tags, we have two sources - one from ingest one from UI (editable). We simply add multiple search fields instead of merging them.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

/**
* List of field descriptions
*/
editedFieldDescriptions: optional array[string]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that fieldDescriptions is coming from SchemaMetadata, whereas editedFieldDescriptions is coming from EditableSchemaMetadata. But what is the source of truth when it comes to field description? If a user has made changes to a field description via UI, shouldn't that be the "updated" field description? Why do we need to keep both?
Not sure if this has been discussed before. If yes, please feel free to point to relevant doc/rfc.

Copy link
Contributor

@shirshanka shirshanka May 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question @jywadhwani
I'm inclined to let this implementation stay for the following reasons:

  1. We should keep the "human written" documentation separate from the "schema-attached" documentation (we are already doing that in the db, so makes sense to continue doing it in the indexes). This way, we don't mix the two streams of information (especially without a governed approval flow for documentation overrides). In fact, we have been asked to surface both the pieces of information in the UI from the community (this is not present in the app currently).
  2. Once we build that sort of approval flow in the application, we can store breadcrumbs for when an editable schema field description overrides an existing schema field description (imagine a flag on the schema-field-description marking it as not-to-be-used), which would trigger the indexer to remove this text from the index.

Hope this addresses your concern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks for the explanation @shirshanka.

Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants