AVRO-3427: skip creation of namespace directories for csharp schema

[kordos]

…of directories for namespace. Just generate classes in output directory

Make sure you have checked all steps below.

Jira

• My PR addresses the following Avro Jira

Tests

Commits

Documentation

[zcsizmadia]

#1574 (if approved and merged) will have many unit tests for checking the generated directory structure. I think it would be great to extend those unit tests with this flag to make sure that the files end up in places wherw it is expected

[zcsizmadia]

Do the other langauges have this capability?

[kordos]

As I'm mainly developing in C# I do not know if any other language have such capability.
I'll dig into unit test soon. I just wanted to create early PR to get some comments ;)

[kordos]

I cannot find unit tests for changed methods: Avro.AvroGen.GenSchema and Avro.CodeGen.WriteTypes. Is it unit/integration tested anywhere?

[zcsizmadia]

Other languages I meant the avrogen tool (e.g. avrogen for c++, ...). If they jave --skip-namespaces flag at all, or something similar, just to match the name of the argument.

[zcsizmadia]

#1574 has a new file lang/csharp/src/apache/test/CodGen/AvroGenTests.cs, which has tests to validate avrogen as an integration test, from a very high level, by calling the tool with command line arguments, compile the schema file, compile all the generated cs files. There are test cases for the matching the generated types and the generated file names.

[kordos]

#1574 has a new file lang/csharp/src/apache/test/CodGen/AvroGenTests.cs, which has tests to validate avrogen as an integration test, from a very high level, by calling the tool with command line arguments, compile the schema file, compile all the generated cs files. There are test cases for the matching the generated types and the generated file names.

Do you know when it will be merged?

[zcsizmadia]

Not sure, however if this is merged sooner, I will make suer to add unit tests to cover new flag with the flattened structure

[kordos]

Other languages I meant the avrogen tool (e.g. avrogen for c++, ...). If they jave --skip-namespaces flag at all, or something similar, just to match the name of the argument.

I checked briefly and I didn't notice any flag like -skip-namespaces used in C++ or java

[kordos]

Not sure, however if this is merged sooner, I will make suer to add unit tests to cover new flag with the flattened structure

Ok, then I'm changing this draft as ready to review. if #1574 will be merged first then I'll add test. If no then unit tests will be added after #1574 will be merged.

[kordos]

@martin-g Could you merge this PR and create a new nuget package from it?

[martin-g]

I have enabled the CI checks.
I will review it later and if everything is OK I can merge it. I was hoping @zcsizmadia or @KyleSchoonover to give their blessing (PR approval) because I am not C# developer and my reviews here are not very reliable! :-)
The nuget will be released once Avro 1.11.1/1.12.0 is released. I am not sure when this will happen!

[kordos]

I have enabled the CI checks. I will review it later and if everything is OK I can merge it. I was hoping @zcsizmadia or @KyleSchoonover to give their blessing (PR approval) because I am not C# developer and my reviews here are not very reliable! :-) The nuget will be released once Avro 1.11.1/1.12.0 is released. I am not sure when this will happen!

Thanks for your answer. Is there a scheduled release of nuget packages? I was thinking that merge to master will create nuget package and the number will be based on actual change and semantic versioning? isn't that the case?

[martin-g]

I was thinking that merge to master will create nuget package and the number will be based on actual change and semantic versioning? isn't that the case?

No. Apache projects are more conservative.
Releases are prepared by a release manager, once in a while.
Then they are voted for at least 3 days. Here the community is encouraged to test the proposed artifacts and report any problems.
If there is a regression then it is being fixed and the release process is restarted.
If no problems are reported then the actual release is executed and an announcement is sent to users@, dev@, ...

[zcsizmadia]

Looks ok, however it is always hard to say so when there are no unit tests. It is not @kordos fault, simply we just dont have coverage for avrogen. I have #1571, which test coverage for avrogen, however it must be reviewed, since there is some code shuffling there. Latr today I will for just testing add this feature to my local branch and validate with unit testing, just to be sure this change has no no unknown side effect. So merging this is not blocked by #1571.

I am not sure how I feel about the name itself. My first reaction to the name if I would see it for the first time is, that the actual namespace to the classes are flattened as well, not just the directory structure. (Maybe -flatten-directories).

[zcsizmadia]

@martin-g Ran some unit tests on the expected directory structure on my future test cases in #1571 and all looks good.

[kordos]

I am not sure how I feel about the name itself. My first reaction to the name if I would see it for the first time is, that the actual namespace to the classes are flattened as well, not just the directory structure. (Maybe -flatten-directories).

I don't mind changing option name. I chose --skip-directories as it was my first idea and I just wanted to start PR process early. It's hard to find some short name that would be descriptive enough for that feature. I'm open for suggestions ;)

[zcsizmadia]

I am just curious, what is the use case for this feature?

[kordos]

I am just curious, what is the use case for this feature?

I described that in jira: https://issues.apache.org/jira/browse/AVRO-3427. Do you need any more info?

[zcsizmadia]

I understand the description, I was just curious what the use case is when a flat directory structure is preferred.

[zcsizmadia]

The reason I am asking is that if you have only 1 or a very few schema files you work with, the chance for name collision is very small or 0. As soon as you have many schemas to compile, like many projects do, the chance for a type name collision is getting higher since you are flattening the directory structure. This is the main reason many C# source codes are following that pattern of matching the namespace with the directory structure.

Of course avrogen wont be really able to detect and report collisions, potentially causing the developer headache to figure out what went wrong, when the generated code does not even compile.

So this was the reason behind my question about what use case you try to cover with this feature ;)

[kordos]

The reason I am asking is that if you have only 1 or a very few schema files you work with, the chance for name collision is very small or 0. As soon as you have many schemas to compile, like many projects do, the chance for a type name collision is getting higher since you are flattening the directory structure. This is the main reason many C# source codes are following that pattern of matching the namespace with the directory structure.

Of course avrogen wont be really able to detect and report collisions, potentially causing the developer headache to figure out what went wrong, when the generated code does not even compile.

So this was the reason behind my question about what use case you try to cover with this feature ;)

Namespace for classes isn't changed and classes are generated with full namespace. Just avrogen is putting them in wrong directory when directories have many namespaces in it (/foo.bar.api/ is a one directory) with current implementation of avrogen I have to generate classes, then manually move them from wrongly generated path to correct path, and manually remove wrong directories. It's very annoying and not the best user experience ;) Tool is not handling multi namespaces directories. In order to avoid that my change generate classes in output directory, without creating of namespace directories. I'm not sure if my clarification helps to understand the problem...

[zcsizmadia]

If the namspace field in the schema definition is foo.bar.ap, the directory structure will be ./foo/bar/ap for that type as of now. Are you saying that avrogen generates a single foo.bar.ap directory? Could you provide an example schema definition, the generated directory structure and your expected directory structure here?

Here is my schema:

{"namespace": "org.apache.avro.codegentest.testdata",
  "type": "record",
  "name": "NestedLogicalTypesRecord",
  "doc" : "Test nested types with logical types in generated Java classes",
  "fields": [
    {
      "name": "nestedRecord",
      "type": {
        "namespace": "org.apache.avro.codegentest.testdata",
        "type": "record",
        "name": "NestedRecord",
        "fields": [
          {
            "name": "nullableDateField",
            "type": ["null", {"type": "int", "logicalType": "date"}]
          }
        ]
      }
    }]
}

avrogen will generate the following directory structure:

./org/apache/avro/codegentest/testdata/NestedLogicalTypesRecord.cs
./org/apache/avro/codegentest/testdata/NestedRecord.cs

[zcsizmadia]

Are you referring this issue, which was fixed in v1.9.0?6eda0b4

What is the avrogen version you are using?

[kordos]

In the future a directory mapping feature might extend avrogen to your use: --directory-alias DIR:ALIAS

So your execution could be: avrogen -s example.avsc . --directory-alias org/apache/avro:org.apache.avro the rest of the namespace sub directories would be automatically generated. Pretty much bfore creating the directory the aliases are replaced in the outputDIr in the WriteTypes function

I like your idea with --directory-alias parameter as it is more flexible and I think it would be nice to introduce that in future.
However as it is a bit more complex solution to develop and use in command line I'd like to stay with my simple option that allow to completely skip creation of directories.

[RyanSkraba]

Hey there -- I haven't been following the C# PRs very closely, but I just proposed to start thinking about 1.11.1 on the mailing list Should this PR be on the radar?

[zcsizmadia]

It can be. Indirectly unit tested it locally, seems to be ok. and the change btw is very trivial.

[zcsizmadia]

@RyanSkraba btw big hurray on the 1.11.1 news ;)

[KyleSchoonover]

@kordos and @zcsizmadia threw together what I was thinking: #1594

[kordos]

Hey there -- I haven't been following the C# PRs very closely, but I just proposed to start thinking about 1.11.1 on the mailing list Should this PR be on the radar?

Would be great if you could squeeze this change to 1.11.1 release :)

[kordos]

@kordos and @zcsizmadia threw together what I was thinking: #1594

Ok, I know what you meant. Idea with two parts parameter is nice however parsing it looks quite complex and it would require good unit test coverage to prove that all that changes work. if we could use some library for parsing command line arguments then it would be a lot simpler.

[zcsizmadia]

@kordos #1595 is merged. Please let me know if you need any help with implementing the unit tests, however it should be fairly trivial,

[kordos]

@kordos #1595 is merged. Please let me know if you need any help with implementing the unit tests, however it should be fairly trivial,

awesome. I'll have a look in my spare time :)

[KyleSchoonover]

@kordos Can you deal with the merge conflicts? We are ready to merge.

[zcsizmadia]

Unit tests should be added before merge

[kordos]

@kordos Can you deal with the merge conflicts? We are ready to merge.

Hi. Lately I didn't have time to work on this. I hope I'll find time soon and I'll add unit tests. I'll let you know if I have any issues with merge.

[kordos]

Unit tests should be added before merge

Merge is done. I'll try to find some time to add unit tests ;)

[kordos]

@zcsizmadia, @KyleSchoonover I added unit tests. Could you review them?

[zcsizmadia]

@martin-g looks ok, but test build did not run yet.

[martin-g]

Thank you, @kordos !