Fix renaming a dimension in aggregations

cdm/core/src/main/java/ucar/nc2/internal/ncml/NcmlReader.java

@@ -810,9 +810,11 @@ private void readDim(Group.Builder groupBuilder, @Nullable Group refGroup, Eleme
 
     String nameInFile = dimElem.getAttributeValue("orgName") != null ? dimElem.getAttributeValue("orgName") : name;
 
-    // LOOK this is wrong, groupBuilder may already have the dimension.
     // see if it already exists
-    Dimension dim = (refGroup == null) ? null : refGroup.findDimension(nameInFile);
+    Optional<Dimension> dimFromAgg = groupBuilder.findDimension(nameInFile);
+    Dimension dim =
+        (refGroup == null || dimFromAgg.isPresent()) ? dimFromAgg.orElse(null) : refGroup.findDimension(nameInFile);
+
     if (dim == null) { // nope - create it
       String lengthS = dimElem.getAttributeValue("length");
       if (lengthS == null) {

cdm/core/src/test/data/ncml/aggregationRenameDim.xml

@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<netcdf xmlns="http://www.unidata.ucar.edu/namespaces/netcdf/ncml-2.2">
+
+  <dimension name="newTime" orgName="time"/>
+  <variable name="newTime" orgName="time" />
+  <variable name="newTime" shape="newTime" />
+  <variable name="P" shape="newTime lat lon" />
+  <variable name="T" shape="newTime lat lon" />
+
+  <aggregation dimName="time" type="joinExisting">
+    <netcdf location="nc/jan.nc"/>
+    <netcdf location="nc/feb.nc"/>
+  </aggregation>
+</netcdf>

cdm/core/src/test/data/ncml/aggregationScanRenameDim.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<netcdf xmlns="http://www.unidata.ucar.edu/namespaces/netcdf/ncml-2.2">
+
+  <dimension name="newTime" orgName="time"/>
+  <variable name="newTime" orgName="time" />
+  <variable name="newTime" shape="newTime" />
+  <variable name="P" shape="newTime lat lon" />
+  <variable name="T" shape="newTime lat lon" />
+
+  <aggregation dimName="time" type="joinExisting">
+    <scan location="nc/" regExp="^(jan.nc|feb.nc)$"/>
+  </aggregation>
+</netcdf>

cdm/core/src/test/java/ucar/nc2/internal/ncml/TestNcmlModifyDim.java

@@ -3,8 +3,6 @@
 import static com.google.common.truth.Truth.assertThat;
 
 import java.io.IOException;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
 import org.junit.Test;
 import ucar.ma2.Array;
 import ucar.ma2.IndexIterator;
@@ -14,44 +12,65 @@
 import ucar.nc2.ncml.TestNcmlRead;
 
 public class TestNcmlModifyDim {
-  private static final String filename = "file:./" + TestNcmlRead.topDir + "modifyDim.xml";
-  private static NetcdfFile ncfile = null;
+  @Test
+  public void shouldRenameDim() throws IOException {
+    final String filename = "file:./" + TestNcmlRead.topDir + "modifyDim.xml";
+
+    try (NetcdfFile ncfile = NcmlReader.readNcml(filename, null, null).build()) {
+      assertThat(ncfile.getRootGroup().getDimensions().size()).isEqualTo(3);
+
+      Dimension newDim = ncfile.findDimension("newTime");
+      assertThat(newDim).isNotNull();
+      assertThat(newDim.isVariableLength()).isFalse();
+      assertThat(newDim.isShared()).isTrue();
+      assertThat(newDim.isUnlimited()).isTrue();
+
+      Dimension oldDim = ncfile.findDimension("time");
+      assertThat(oldDim).isNull();
+
+      Variable time = ncfile.findVariable("time");
+      assertThat((Object) time).isNotNull();
+      assertThat(time.getDimensionsString()).isEqualTo("newTime");
+
+      Array data = time.read();
+      assertThat(data.getRank()).isEqualTo(1);
+      assertThat(data.getSize()).isEqualTo(4);
+      assertThat(data.getShape()[0]).isEqualTo(4);
+      assertThat(data.getElementType()).isEqualTo(int.class);
 
-  @BeforeClass
-  public static void setUp() throws IOException {
-    ncfile = NcmlReader.readNcml(filename, null, null).build();
+      IndexIterator dataIter = data.getIndexIterator();
+      assertThat(dataIter.getIntNext()).isEqualTo(6);
+      assertThat(dataIter.getIntNext()).isEqualTo(18);
+    }
   }
 
-  @AfterClass
-  public static void tearDown() throws IOException {
-    ncfile.close();
+  @Test
+  public void shouldRenameDimInAggregation() throws IOException {
+    final String filename = "file:./" + TestNcmlRead.topDir + "aggregationRenameDim.xml";
+    checkDimIsRenamed(filename);
   }
 
   @Test
-  public void shouldRenameDim() throws IOException {
-    assertThat(ncfile.getRootGroup().getDimensions().size()).isEqualTo(3);
-
-    Dimension newDim = ncfile.findDimension("newTime");
-    assertThat(newDim).isNotNull();
-    assertThat(newDim.isVariableLength()).isFalse();
-    assertThat(newDim.isShared()).isTrue();
-    assertThat(newDim.isUnlimited()).isTrue();
-
-    Dimension oldDim = ncfile.findDimension("time");
-    assertThat(oldDim).isNull();
-
-    Variable time = ncfile.findVariable("time");
-    assertThat((Object) time).isNotNull();
-    assertThat(time.getDimensionsString()).isEqualTo("newTime");
-
-    Array data = time.read();
-    assertThat(data.getRank()).isEqualTo(1);
-    assertThat(data.getSize()).isEqualTo(4);
-    assertThat(data.getShape()[0]).isEqualTo(4);
-    assertThat(data.getElementType()).isEqualTo(int.class);
-
-    IndexIterator dataIter = data.getIndexIterator();
-    assertThat(dataIter.getIntNext()).isEqualTo(6);
-    assertThat(dataIter.getIntNext()).isEqualTo(18);
+  public void shouldRenameDimInAggregationScan() throws IOException {
+    final String filename = "file:./" + TestNcmlRead.topDir + "aggregationScanRenameDim.xml";
+    checkDimIsRenamed(filename);
+  }
+
+  private void checkDimIsRenamed(String filename) throws IOException {
+    try (NetcdfFile ncfile = NcmlReader.readNcml(filename, null, null).build()) {
+      Dimension newDim = ncfile.findDimension("newTime");
+      assertThat(newDim).isNotNull();
+
+      Dimension oldDim = ncfile.findDimension("time");
+      assertThat(oldDim).isNull();
+
+      Variable newTime = ncfile.findVariable("newTime");
+      assertThat((Object) newTime).isNotNull();
+      Array data = newTime.read();
+      assertThat(data.getSize()).isEqualTo(59);
+
+      Variable time = ncfile.findVariable("time");
+      assertThat((Object) time).isNull();
+    }
   }
 }