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

Support for runtime_mapping #2164

Open
coreation opened this issue May 16, 2023 · 6 comments
Open

Support for runtime_mapping #2164

coreation opened this issue May 16, 2023 · 6 comments
Labels

Comments

@coreation
Copy link
Contributor

coreation commented May 16, 2023

Hi everyone,

I was looking into the Script Query part of ElasticSearch and was wondering if the "runtime_mappings" clause was supported by the script field.

I see that the first example on the documentation page is possible using Script but since that is inherits from the AbstractQuery class, I don't think this can be used to create runtime fields.

Am I correct to say that this functionality is not available in this library; if so would that require a new folder + implementation to be made since this is a different clause standing next to the fields / query top level query parts of an ElasticSearch request?

Edit: I assume you could reach the desired result by making a Query() object and injecting the runtime_mappings raw query as an array in there.

@ruflin
Copy link
Owner

ruflin commented May 22, 2023

As you described above, you should be able to use it with this lib by just using query or inject raw json.

But we should add this feature to the lib. Many things inside runtime_mappings are similar / same as inside the mapping part. If this is the case, do we really need a new folder or could one top level class or just a method deal with it?

@coreation
Copy link
Contributor Author

Hi @ruflin thank you for the feedback!

If I'm thinking in terms of domains, I would create something (file/folder) in the Query folder, because it's part of the Query DSL. I'm not that familiar with the build up of the library but based on what I know is that we could add it in the Query folder.

Given that this is a separate part of the ElasticSearch "query", I'm wondering if this would turn out to be a new DSL interface type. If that would be the case, then this would result in the same folder structure as this library does with the other interface types: src > RuntimeMappings > Query / Script /....

Could you perhaps elaborate on your solution at then end of your comment, because I'm too unfamiliar probably to follow your train of thought.

@ruflin
Copy link
Owner

ruflin commented May 25, 2023

You described the two approaches I had in mind really well. Conceptually I agree, file / folder in Query would make more sense but that is not the path that Elasticsearch went down. As I assume, in most cases users will look at the Elasticsearch docs and then come to Elastica and try to reproduce it, so your second suggestion fits better.

The part I was wondering, if it might be enough to have src > RuntimeMappings.php. RuntimeMappings can be added to Search and RuntimeMappings->addMappings(Mapping) or similar exists. My current assumption is, that we don't the interface and specific sub classes, because it is all already covered by existing classes that are added to RuntimeMappings. But have not checked in enough detail the inner structure of runtime_mappings to be sure about this.

@coreation coreation changed the title (Question) Is the runtime_mappings clause supported? Support for runtime_mapping May 25, 2023
@coreation
Copy link
Contributor Author

Hey @ruflin I'm happy to see I was on track with my train of thought. I'd agree with not making a folder just to store the one class inside, since there's only 1 option.

I'm currently quite occupied to getting to a PR might take some time. What I'll do is change the title, if you could add the label "Feature" that'd be great, as I don't wield that kind of power ;).

For whoever picks this up, the approach would be to:

  • added a new class RuntimeMappings.php in the src folder
  • the Search class must be able to accept a RuntimeMappings object, similar to how you pass a Query object to a Search object
  • What needs to be investigated is what is supported in the runtime_mappings ElasticSearch structure so that we can apply the same restrictions in the RuntimeMappings class

@ruflin can I make the assumption that in a first iteration we could leave it quite open, and allow the user to pass the runtime_mapping as an array constructor argument? This then does not fall far from how you can use it now using the Query class, but at least it's a step in the right direction. Looking forward to how you see an acceptable PR being made.

@ruflin ruflin added the feature label May 25, 2023
@ruflin
Copy link
Owner

ruflin commented May 25, 2023

@ruflin can I make the assumption that in a first iteration we could leave it quite open, and allow the user to pass the runtime_mapping as an array constructor argument? This then does not fall far from how you can use it now using the Query class, but at least it's a step in the right direction. Looking forward to how you see an acceptable PR being made.

I think we need the investigation on your point 3 to make the call if pure array is enough to get started. I expect we eventually end up with something like we have for query (https://github.com/ruflin/Elastica/blob/8.x/src/Query.php#L86) which supports 4 different types because it grew over time. If we already can figure out the right type in the beginning, I don't mind. But I rather have the feature then iterate for a year on the perfect solution.

I expect as soon as we have a PR and some tests, the decisions become much more obvious.

@coreation
Copy link
Contributor Author

Ok, that's clear! I'll see when or if I can pick this up. First task is as you mentioned, see what the runtime_mappings support in ElasticSearch and compare that with that the Elastica library already wraps. If that's a close match, we can go for strict typing otherwise it'll be something along the lines of the Query constructor.

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

No branches or pull requests

2 participants