Skip to content

Conversation

rursprung
Copy link
Contributor

@rursprung rursprung commented Apr 14, 2025

Description

alternative version of #443 from @reta which shows the failing test. feel free to take that commit over into the original PR and close this PR here instead.

Note that one of the test cases currently fails and requires opensearch-project/opensearch-java#1513 to be resolved first, thus it has been disabled for the OSC client.

Issues Resolved

n/a

Check List

  • New functionality includes testing.
    • All tests pass (the whole point is to show a failing test; though it's disabled, so technically it passes CI)
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

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.

}

@Test
@DisabledIf(value = "isOSCTest", disabledReason = "https://github.com/opensearch-project/opensearch-java/issues/1513")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove this to see the test failing

Note that one of the test cases currently fails and requires
opensearch-java#1513 to be resolved first, thus it has been disabled for
the OSC client.

Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: Ralph Ursprung <[email protected]>
@rursprung rursprung force-pushed the add-dynamic_templates-test-case branch from 7b58d7a to a4520c7 Compare April 14, 2025 07:40
reta added a commit to reta/spring-data-opensearch that referenced this pull request Apr 14, 2025
@reta
Copy link
Collaborator

reta commented Apr 14, 2025

@rursprung this is expected behavior, the values with dots are not accepted to object fields (but to nested or flattened fields), changes goes back to https://www.elastic.co/guide/en/elasticsearch/reference/2.4/dots-in-names.html

@rursprung
Copy link
Contributor Author

rursprung commented Apr 15, 2025

@rursprung this is expected behavior, the values with dots are not accepted to object fields (but to nested or flattened fields), changes goes back to https://www.elastic.co/guide/en/elasticsearch/reference/2.4/dots-in-names.html

the whole goal here is to have this split up - i.e. indexing A.B should create a field B nested within A; this is also what the documentation which you linked to suggests? and with the old client this worked perfectly fine (see also that the test succeeds with the ORHLC).

@reta
Copy link
Collaborator

reta commented Apr 16, 2025

the whole goal here is to have this split up - i.e. indexing A.B should create a field B nested within A; this is also what the documentation which you linked to suggests? and with the old client this worked perfectly fine (see also that the test succeeds with the ORHLC).

OK, so it is really very tricky, I think the case I run into is:

// First doc

	"names" : {
		"a" : "c"
	}
}

// Second doc

	"names" : {
		"a.b" : "c"
	}
}

So this does not work obviously. To your point, the general exception that dots are expanded into properties for object types is correct, but the presence of "type": "object" (dynamic template) in case of OpenSearch Java client does break the expecations

@rursprung
Copy link
Contributor Author

// First doc

this should be:

	"names" : {
		"a" : {
			"b" : "c"
		}
	}
}

So this does not work obviously. To your point, the general exception that dots are expanded into properties for object types is correct, but the presence of "type": "object" (dynamic template) in case of OpenSearch Java client does break the expecations

exactly, that's the problem and why i've reported it to opensearch-java as discussed: opensearch-project/opensearch-java#1513

but i'm not familiar at all with the code generation going on there, so i don't know how to get this fixed there (i guess either the source from which it is being generated is wrong or the generator has a bug?)

@reta
Copy link
Collaborator

reta commented Apr 16, 2025

@rursprung closing this one, #443 has a reproducer now, thank you

@reta reta closed this Apr 16, 2025
@rursprung rursprung deleted the add-dynamic_templates-test-case branch April 16, 2025 14:40
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.

2 participants