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.

Sign up for GitHub

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

Jump to bottom

Fix bug for not able to get sourceTables from metadata #883

Merged
merged 4 commits into from
Nov 12, 2024
Merged

Fix bug for not able to get sourceTables from metadata #883

merged 4 commits into from
Nov 12, 2024

Conversation

seankao-az
Copy link
Collaborator

@seankao-az seankao-az commented Nov 8, 2024 •
edited
Loading

Description

Using custom implementation for FlintIndexMetadataService exposes a bug where we incorrectly get empty sourceTables from FlintMetadata.
Depending on how the FlintMetadata is generated, the metadata.properties.get("sourceTables") could either be of type java array or scala array.
This PR fixes the bug, now considering both cases.

Also added some logs for troubleshooting in production environment.

Related Issues

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@seankao-az
add logs
137e5a8 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao-az
match Array when reading sourceTables
dd4535b 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao-az
add test cases
efd2207 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao-az seankao-az self-assigned this Nov 8, 2024
@seankao-az seankao-az marked this pull request as ready for review November 8, 2024 23:09
ykmr1224
ykmr1224 reviewed Nov 11, 2024
View reviewed changes
...rk-integration/src/main/scala/org/opensearch/flint/spark/mv/FlintSparkMaterializedView.scala Outdated
*/
def getSourceTablesFromMetadata(metadata: FlintMetadata): Array[String] = {
logInfo(s"Getting source tables from metadata $metadata")
metadata.properties.get("sourceTables") match {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can metadata.properties to be null?

Copy link
Collaborator Author

@seankao-az seankao-az Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

no
FlintMetadata is constructed with an empty properties map as member when there isn't any property

dai-chen
dai-chen reviewed Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

Is it possible to enforce a single and language-independent format in metadata, e.g. JSON array?

@seankao-az
Copy link
Collaborator Author

Is it possible to enforce a single and language-independent format in metadata, e.g. JSON array?

Not quite sure..
Since when reading from OpenSearch index mappings, we use jackson library to deserialize into FlintMetadata, and jackson library will use java.util.ArrayList. I'll try using this type instead.

Or do you mean using json string? Like

{"sourceTables": "[\"a.b.c\"], [\"d.e.f\"]"}

@dai-chen
Copy link
Collaborator

Is it possible to enforce a single and language-independent format in metadata, e.g. JSON array?

Not quite sure.. Since when reading from OpenSearch index mappings, we use jackson library to deserialize into FlintMetadata, and jackson library will use java.util.ArrayList. I'll try using this type instead.

Or do you mean using json string? Like

{"sourceTables": "[\"a.b.c\"], [\"d.e.f\"]"}

Yeah, just trying to understand where this problem comes from? Is Scala or Java array stored in metadata?

@seankao-az
use ArrayList only
ce5fea8 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao-az seankao-az force-pushed the empty-source-tables-fix branch from 76c9e0e to ce5fea8 Compare November 12, 2024 17:02
@seankao-az
Copy link
Collaborator Author

mainly comes from this line of scala code. I was putting scala Array[String] here.
https://github.com/opensearch-project/opensearch-spark/pull/883/files#diff-1e6cbb4cde65f1142862f3b539a438d534fd70a229499efd671ef9aae67ff29fR75

Now I've changed it to use ArrayList. so for both cases (mv.metadata() and deserialized metadata from index mapping) will have ArrayList. Do you think this is better? @dai-chen

@seankao-az seankao-az merged commit dd9c0cf into opensearch-project:main Nov 12, 2024
4 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 12, 2024
@github-actions
Fix bug for not able to get sourceTables from metadata (#883)
20f21d6 
* add logs

Signed-off-by: Sean Kao <seankao@amazon.com>

* match Array when reading sourceTables

Signed-off-by: Sean Kao <seankao@amazon.com>

* add test cases

Signed-off-by: Sean Kao <seankao@amazon.com>

* use ArrayList only

Signed-off-by: Sean Kao <seankao@amazon.com>

---------

Signed-off-by: Sean Kao <seankao@amazon.com>
(cherry picked from commit dd9c0cf)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Nov 12, 2024
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 12, 2024
@github-actions
Fix bug for not able to get sourceTables from metadata (#883)
ed0eebd 
* add logs

Signed-off-by: Sean Kao <seankao@amazon.com>

* match Array when reading sourceTables

Signed-off-by: Sean Kao <seankao@amazon.com>

* add test cases

Signed-off-by: Sean Kao <seankao@amazon.com>

* use ArrayList only

Signed-off-by: Sean Kao <seankao@amazon.com>

---------

Signed-off-by: Sean Kao <seankao@amazon.com>
(cherry picked from commit dd9c0cf)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Nov 12, 2024
Merged
seankao-az pushed a commit that referenced this pull request Nov 12, 2024
@opensearch-trigger-bot @github-actions
Fix bug for not able to get sourceTables from metadata (#883) (#893)
a92f7f6 
* add logs



* match Array when reading sourceTables



* add test cases



* use ArrayList only



---------


(cherry picked from commit dd9c0cf)

Signed-off-by: Sean Kao <seankao@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
saranrajnk pushed a commit to saranrajnk/opensearch-spark that referenced this pull request Nov 12, 2024
@opensearch-trigger-bot @github-actions
Fix bug for not able to get sourceTables from metadata (opensearch-pr…
ecc8752 
…oject#883) (opensearch-project#893)

* add logs



* match Array when reading sourceTables



* add test cases



* use ArrayList only



---------


(cherry picked from commit dd9c0cf)

Signed-off-by: Sean Kao <seankao@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
seankao-az pushed a commit that referenced this pull request Nov 13, 2024
@opensearch-trigger-bot @github-actions
Fix bug for not able to get sourceTables from metadata (#883) (#894)
05ad2b2 
* add logs



* match Array when reading sourceTables



* add test cases



* use ArrayList only



---------


(cherry picked from commit dd9c0cf)

Signed-off-by: Sean Kao <seankao@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kenrickyap pushed a commit to Bit-Quill/opensearch-spark that referenced this pull request Dec 11, 2024
@seankao-az @14yapkc1
Fix bug for not able to get sourceTables from metadata (opensearch-pr…
0f08cfa 
…oject#883)

* add logs

Signed-off-by: Sean Kao <seankao@amazon.com>

* match Array when reading sourceTables

Signed-off-by: Sean Kao <seankao@amazon.com>

* add test cases

Signed-off-by: Sean Kao <seankao@amazon.com>

* use ArrayList only

Signed-off-by: Sean Kao <seankao@amazon.com>

---------

Signed-off-by: Sean Kao <seankao@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ykmr1224 ykmr1224 ykmr1224 left review comments

@dai-chen dai-chen dai-chen approved these changes

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@vamsimanohar vamsimanohar Awaiting requested review from vamsimanohar

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@kaituo kaituo Awaiting requested review from kaituo kaituo is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@noCharger noCharger Awaiting requested review from noCharger noCharger is a code owner

@LantaoJin LantaoJin Awaiting requested review from LantaoJin LantaoJin is a code owner

Assignees

@seankao-az seankao-az

Labels
0.7 backport 0.5-nexus backport 0.6 bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seankao-az @dai-chen @ykmr1224